-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(re)implement equality with 2pi #22
Comments
This would be an act of type piracy of types defined in Julia's standard library, which is considered bad practice: https://docs.julialang.org/en/latest/manual/style-guide.html#Avoid-type-piracy-1 I don't see why it's a problem that |
I understand the concern, which I agree is serious in the generic case (e.g. if done for types that are completely distinct from each other) but given auto-promotion rules for Irrationals, I don't see how treating 2pi as an Irrational could cause real-world problems. Aside from the conceptual "sin" (which has to be balanced with the IMO more serious mathematical "sin" of defining tau != 2pi), what kind of problems do you foresee for users of this package? |
If I have a function:
then the result will be type unstable. |
I understand that there will be type instability, but can it actually cause bugs in this particular case, or only performance degradation? I mean, I don't think there's any functional distinction between, say, using Otherwise, would the |
Most of performance of Julia codes comes from writing type-stable code. Disrupting this principle in a simple (and also common) operation like
Making |
You certainly gave me food for thought. After mulling this over, I think I'll agree that we shouldn't make See, I was hoping one could do With that in mind, though, I think it would be nice to provide some sort of warning for when a downstream package uses 2pi in its code -- which could be done by overriding If that distinction is not possible (or the warning approach found too intrusive), then an alternative could be a helper script or method in this package that would process a given module or file and output a list of locations where 2pi could be replaced by tau. This is easy enough to do by hand, but depending on the search method, one could easily miss valid instances -- What do you think? |
Even if type piracy weren't a concern (but it is)...
...with which goal? Sorry, I don't see what you expect to gain with such auto-promotion.
I don't know, I don't have a strong opinion on this. In any case, I wouldn't make it part of the package itself, at least an extra script to be placed outside |
I'm afraid I didn't make myself clear. I am already on board with not doing the auto-promotion. I was just explaining what was my thinking for proposing that initially, not advocating for that -- which I meant to make clear when I said:
I edited my comment above to hopefully clarify I was talking about the users of Tau.jl making the 2pi --> tau change, rather than Tau.jl doing that under the hood. Also, I didn't mean to imply that I don't believe type piracy is a serious concern, just that I tend to value users expectations of a tool (in this case that
Ok. I'll open a separate issue to track the creation of such a script. |
Issue created at #24. |
Good, much better now :-) |
For future reference, a similar possibility was also raised by @martinholters in JuliaLang/julia#7994 (comment):
|
How is an "irrational 1" better than the integer |
Agreed. I wasn't arguing for that particular idea, just making a cross-reference for posterity, since it also mentions the potentially unexpected inequality arising from multiplying an |
Yes, to clarify: My point was that presently, there is no multiplicative identity for |
I have been collecting some loose notes about this but hadn't posted a plan publicly as I didn't have a clear plan of how to go about tackling this. But given @giordano's recent involvement with the package (and with the overall handling of
Irrational
in julia), I feel the time is right to move ahead and make a decision.As the README indicates, this package was originally meant to provide the
tau == 2pi
equality (which should by definition be true). This is not possible at the moment due to the*
operator implicit in 2pi converting the result of that expression to a float, which Irrationals are explicitly defined as not equal to. However, since this package was created explicitly to give special meaning to the value of 2pi, it would make sense for it to ensure that 2pi handles as the same Irrational as well. This may not be in scope for base Julia (@StefanKarpinski often expresses his concern that at some point these changes will drift towards implementing a full-fledged math parsing engine or CAS), but it sounds reasonably in scope for a package like this.One way of doing so could be using Julia's core feature of multiple dispatch to override the * operator for the case where the operands are pi and 2, as I suggested a while ago in this thread. There is even some precedent for such special cases in base julia: exp.jl, for instance, has a special case for exp(1), implemented using a simple
if
test.Instead of an if test, I have been looking at Value types to implement this. PatternDispatch could also provide inspiration for possible alternative approaches. These options may be overkill for the single 2pi case, but may be adequate if we want something more generic.
Indeed, a more general solution could be more appropriate: integer multiples of pi (or tau) should, for semantic reasons, be handled with the same manner as pi/tau itself. @simonbyrne suggested such a
PiMultiple
type in the PR that introduced sinpi/cospi, and I think the Tau package could make that experiment with aTauMultiple
type.Ok, now that I made the case, I'm curious about any arguments against. In particular, I'm wondering why @giordano considers that "it's exactly the fact that tau isn't the same as
2 * pi
that makes this package particularly useful."The text was updated successfully, but these errors were encountered: