Skip to content

Conversation

oscardssmith
Copy link
Member

My preferred solution to #51141. Hopefully no one is relying on the bad rounding modes, but this definitely needs a pkgeval to confirm. @Seelengrab would this be OK with you? I believe the remaining rounding modes for the single argument round are ones that are relatively mathematically justified.

@oscardssmith oscardssmith added breaking This change will break code needs tests Unit tests are required for this change DO NOT MERGE Do not merge this PR! minor change Marginal behavior change acceptable for a minor release needs pkgeval Tests for all registered packages should be run with this change labels Dec 11, 2023
@Seelengrab
Copy link
Contributor

No, because the remaining ones also aren't justified. The notions described in the docstrings of the rounding modes for Real numbers don't extend to complex numbers, because the tiebreakers listed there don't resolve the ambiguities at all. At the very least, they aren't even consistent with what iseven(::Complex) returns, which ignores the imaginary part entirely.

@nsajko
Copy link
Member

nsajko commented Dec 11, 2023

IMO Sukera's approach is better, but, in case this approach gets accepted instead, I think it would be much better for round to accept a single ComplexRoundingMode value which would be parameterized by two rounding modes, instead of taking two separate RoundingModes.

@oscardssmith
Copy link
Member Author

I think it would be much better for round to accept a single ComplexRoundingMode value which would be parameterized by two rounding modes, instead of taking two separate RoundingModes.

This PR doesn't add the ability for round to take separate rounding modes. That functionality is 9 years old (see a36297e)

@LilithHafner
Copy link
Member

I think #51192 & doc build failures combine to prevent nanosoldier from running on this PR as is.

The concept of "nearest" makes sense for complex numbers.

The concepts of "toward zero" and "away from zero" make sense as tie breakers, though when used alone we have to define their behavior explicitly elementwise (magnitude of differences are insufficient). For example, if we were to define round(x, RoundToZero) as "the Gaussian integer y with smallest magnitude such that |x - y| < 1" then we would get the wrong answer for round(1.9 + 1.9im, RoundToZero)

"Toward even" is iffy, but rounding tiebreakers are imo significantly less important than the primary round/floor/ceil distinction. And it may be too breaking to remove round(::Complex). Which point is "more even" when rounding 0.5 + 2.5im---is it 1 + 3im or 0 + 2im (we currently pick 0 + 2im)? And what about 0.5 + 1im is 0 + 1im or 1 + 1im more even? We currently pick 0 + 1 im.

@Seelengrab
Copy link
Contributor

Seelengrab commented Dec 16, 2023

rounding tiebreakers are imo significantly less important than the primary round/floor/ceil distinction.

The whole issue is that you can't extend the notion of rounding elementwise cleanly to floor/ceil. I don't have issues with round(::Complex); that can do whatever it wants. I do have an issue with floor(x::Complex) = round(x, ...) by lieu of a fallback that breaks desirable identities for floor.

@oscardssmith oscardssmith added this to the 1.11 milestone Dec 21, 2023
@adienes
Copy link
Member

adienes commented Dec 21, 2023

The whole issue is that you can't extend the notion of rounding elementwise cleanly to floor/ceil

to be honest, I don't really agree with this.

sure, the APL definition sets out some nice declarative properties that floor holds on the real number line, but I have yet to see compelling justification as to why these properties matter

I think rounding elementwise honestly feels pretty intuitive for rounding to any lattice in any coordinate space, and is a perfectly reasonable definition, even if some properties only hold when the lattice is one dimensional

I mean, Complex <: Number, and yet complex numbers cannot be totally ordered (as a field), but I wouldn't say it's true that all Number fields must be totally ordered or else it's mathematically iffy...

I think the equivalence of floor and ceil with the rounding modes is pretty nice, and my vote is to not merge this pr

@LilithHafner
Copy link
Member

I think the equivalence of floor and ceil with the rounding modes is pretty nice

This PR keeps that equivalence by making ceil(::Complex) and round(::Complex, RoundUp) both throw.

I think rounding elementwise honestly feels pretty intuitive for rounding to any lattice in any coordinate space, and is a perfectly reasonable definition, even if some properties only hold when the lattice is one dimensional

That seems reasonable. I also don't see any major problems with elementwise floor, though if it's non-disruptive to remove it that's fine by me as well.

if rr in (RoundNearest, RoundFromZero, RoundToZero, RoundNearestTiesAway)
return round(z, rr, rr; kwargs...)
end
error("Rounding Mode $rr not supported for Complex numbers. Use round(z, $rr, $rr)")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would throw and ArgumentError here.

@StefanKarpinski
Copy link
Member

I think the equivalence of floor and ceil with the rounding modes is pretty nice

This PR keeps that equivalence by making ceil(::Complex) and round(::Complex, RoundUp) both throw.

Yes, the whole point of this PR is to bring floor(z) and ceil(z) into correspondence with round(z, RoundDown) and round(z, RoundUp) respectively. However, I'm also ok with just applying all rounding operations "elementwise" to complex numbers.

@oscardssmith oscardssmith removed needs tests Unit tests are required for this change needs pkgeval Tests for all registered packages should be run with this change labels May 23, 2024
@oscardssmith oscardssmith removed this from the 1.11 milestone May 23, 2024
@LilithHafner
Copy link
Member

Triage thinks that it is okay to define floor(::Complex) elementwise. Any choice is arbitrary, and the elementwise arbitrary choice seems better than all the alternatives. If someone asks for the floor of a Complex number, we can give them a reasonable answer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code DO NOT MERGE Do not merge this PR! minor change Marginal behavior change acceptable for a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants