-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Disallow ceil
/trunc
/floor
on complex arguments
#51141
Conversation
See JuliaLang#42060 for why this is not easy to define, and hence is (for now) better left to error, instead of working through a fallback.
This is breaking, or? |
No. All existing releases already throw a MethodError here, the refactor in #50812 just happened to make this work, without taking the implications on #42060 into account. Existing working code should not be affected at all. This PR just improves on the error message and "keeps it broken" (which arguably should have been done in the API refactor..). |
If we were doing it all over, I would prefer to have neither julia> round(1.1 + 1.2im, RoundUp)
2.0 + 2.0im
julia> round(1.1 + 1.2im, RoundDown)
1.0 + 1.0im
julia> round(1.1 + 1.2im, RoundToZero)
1.0 + 1.0im
julia> round(1.1 + 1.2im, RoundNearest)
1.0 + 1.0im
julia> round(1.1 + 1.2im)
1.0 + 1.0im
julia> versioninfo()
Julia Version 1.9.3
Commit bed2cd540a1 (2023-08-24 14:43 UTC)
Build Info:
Official https://julialang.org/ release
Platform Info:
OS: macOS (arm64-apple-darwin22.4.0)
CPU: 8 × Apple M2
WORD_SIZE: 64
LIBM: libopenlibm
LLVM: libLLVM-14.0.6 (ORCJIT, apple-m1)
Threads: 1 on 4 virtual cores
Environment:
JULIA_EDITOR = code I don't see much distinction between |
I agree that single or zero-arg The issue with Considering these difficulties, it's IMO better to just keep this as an error for now. |
CI is green - what a wonderful thing 🎉 |
Shouldn't this just be merged? Also to get PkgEval. But after, or even before backport to 1.10? It would be bad is something defined in 1.10.0 went away in later version. If this turns out to be bad to (not) define, then it can be undone later. It seems we don't want to define this, since problematic mathematically: |
|
We've already picked the elementwise semantic for rounding complex numbers. A reasonable choice IMO, and it's certainly too late to revise that before 2.0. |
No, you unilaterally happened to make I'll label this |
We're committed to (analogous for |
I'm not sure this is wise. You could say people asked for this, and document the definition used. But you might not get the nearest integer number, at least with Float16. Its maxintfloat(Float16) is only 2048, and very small for Float8 (in a package). For minifloat it's only 16, I think Float8 might be the same, there might be more than one definition. It seems odd to special-case round/ceil of Complex to return Float64 (or Float32 ok?). Or just leave out Float16. It might seem odd to some that those do not work on some or any Complex type, but it's justified, and could be done by throwing an error? |
That's not what's happening; the current definition for |
I know it's not happening, I mean complex based on Float16 returning same type seems like a problem, and special casing it to return a different larger type (while NOT type-unstable) would be surprising workaround. |
Triage says that we should keep Three possibilities for floor and ceil:
We should see what PkgEval has to say about options 2 & 3. |
I feel like what this PR does has been misrepresented in triage. I couldn't join because it was at 2:30 AM my time. Has there been any discussion whatsoever about the implications of keeping these methods has on #42060? This is not a matter of whether PkgEval agrees or not, this is about what |
To be more explicit about it, the rounding modes that were deemed to make sense for complex numbers are are ones that don't involve any nothing of "up" or "down", i.e.:
The other ones depend on some notion of "up" and "down" and therefore should not work for complex numbers. There were basically two sensible end situations that triage felt this could end up in:
To avoid breaking things, we would deprecate any "disallowed" methods that already exist rather than deleting pre-existing methods. Any allowed methods would be implemented. In the restrictive option, top-level rounding functions would be allowed iff the corresponding method
@Seelengrab, if I'm understanding you correctly, you're advocating for a third option, where |
Not quite. In an ideal world, we wouldn't have The idea that As such, my objection to the current state on master (and reason for this PR) is that if we choose the current (bad)
This is wrong - all of these depend on a notion of "up" and "down". For example, for Similarly,
|
I also want to repeat that this PR doesn't break any existing/released code. There is no current release where
is a different matter. |
Triage's point is that
yes. The way to interpret To be clear, Triage agrees that Also, if we can't reach consensus on this before next triage, we can talk about it then since next triage is at a better time for you. |
I agree with that! I'm fully on board with removing one-arg All this PR does is explicitly remove the as-of-yet unreleased equivalence of
Then why not merge this PR, which literally only disallows these forwarding methods? I don't understand why this is controversial. Also, I feel like noone on triage has read the literature I've linked multiple times now, which precisely defines what it means to be a
I disagree - both Is there some mathematical disagreement triage has with that literature that I'm missing?
That sounds even more of an argument that the API redesign, while nice conceptually, does not work out well in practice. |
The apl definition is interesting, although I'm not sure how important some of their criteria are (specifically the requirement of distance<1 seems odd since it would imply that distance<.5 needs to be preserved for The key point from triage is that all the problems of |
That again is only possible in the "number line"-like interpretation of a number. That requirement for
I don't think that is worth a whole lot, see https://www.johndcook.com/blog/2021/08/03/complex-floor/ which talks about this (which I ALSO linked in #42060 here...). This is not clear cut, and just pointing to Mathematica is not a good "source of truth". Note:
So it seems to me that the Mathematica definition is not at all common, whereas the one from APL is shared J and others.
Sure, but again, that doesn't mean we should make Also, it feels VERY wrong to introduce a new |
no, this comes directly from the APL paper "3. Fractionality: The magnitude of the difference of a number and its floor shall be less than one". This is the key requirement that they use to define the
The reason to do this is that it is a method that logically exists (by the correspondence between |
Which implies nothing whatsoever about https://www.johndcook.com/blog/2021/08/03/complex-floor/
"Taking the magnitude" is a projection from the vector space to a number line. All vector space properties are lost in that projection; you are only left with an ordinary, orderable, Real number. The APL paper argues that it is useful to keep this fractionality property for
Good thing that the APL paper is not arguing for that then, but for
There is no correspondence between
|
Having first heard of this PR when it was mentioned in Triage, the complexity of complex rounding has been rattling around my mind a bit since. This has prompted the thought that perhaps we can gain some insight/perspective by considering an even more complex class of numbers: quaternions. A quick search turned up an interesting document by Bob Smith (who seems to have been quite involved with APL): Hypercomplex GCD in APL. This document links to an ACM paper published after McDonnell's complex floor titled Complex floor revisited. I found the conclusion of that paper quite insightful, perhaps it might help with the discussion here?
|
To do a little translation of the abbreviations from that paper:
So this recommendation is basically he prefers component-wise floor (FF) and things APL should adopt it for complex numbers, contrary to what it does. He also believes that complex/quaternion truncation (HF) is a useful primitive that should be exposed. Back to McDonnell's APL paper: the "fractionality" criterion strikes me as very poorly motivated. Of course, the name sound good—yes, of course, the difference between a value and its floor should be fractional! And on the real number line that means it should have magnitude less than one. But blindly applying the "magnitude less than one" to the complex plane doesn't really make any sense. It means that the numbers that can be considered fractional in ℂ are the ones inside the unit circle. But contrary to that, I would consider any complex number with both real and imaginary parts less than one to be fractional, which is a very different criterion. If I had started with criteria that led me to such a frankly odd floor function, I would go back and reconsider my criteria. Citing J as an independent data point supporting APL's floor behavior is a bit disingenuous—J and K after it are just APL clones with syntax that you can type on a qwerty keyboard. This weird floor behavior seems to have originated in APL with one person who thought it was a good idea and been regretted in hind sight. |
Since it seems to (again) have been lost in the discussion above, my reason for opening this PR was to make As it turns out, Hypercomplex GCD in APL also has this to say:
(the definitions referred to here are MF & HF) which tracks completely with the stance I've held in these threads so far. This even extends to higher dimensions:
This is very reminiscent of the sphere packing paradoxon in higher dimensions, where with high enough dimension, packing spheres into a unit cube can result in spheres with a larger volume that than unit cube. I don't know why everyone keeps talking as if I just want the APL version. I don't particularly want the APL version (though if we were going for it or any other complex floor, I'd still want to check what exactly we gain and what properties hold), and I've since read enough about this to convince me that we shouldn't have a generic |
See #42060 for why this is not easy to define, and hence is (for now) better left to error, instead of working through a fallback.
This also rewords the docstring of the existing
round
function with two rounding modes, to make it clear that the imaginary rounding mode defaults to the same as the one that's set for the real part.