-
Notifications
You must be signed in to change notification settings - Fork 427
Split complex nan-containing values into two components for IO and casts #27011
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
Draft
bradcray
wants to merge
11
commits into
chapel-lang:main
Choose a base branch
from
bradcray:complex-nan
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+179
−39
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
--- Signed-off-by: Brad Chamberlain <[email protected]>
I realized I should add some tests of complex values who were just nan in one component or the other and found that the string-to-complex casts didn't work in the event that the imaginary portion was nan. But now I think this was another instance of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48760 that could be fixed using the same technique as in chapel-lang#24184, so I did that and then added a test to lock in the behavior, which didn't work prior to this commit. --- Signed-off-by: Brad Chamberlain <[email protected]>
--- Signed-off-by: Brad Chamberlain <[email protected]>
--- Signed-off-by: Brad Chamberlain <[email protected]>
Prior to this, I was finding that the output would differ when compiled on my Mac with clang vs. on linux with gcc, where the 'nani' term would wiggle between '+' (on Mac) and '-' (on Linux). My understanding is that nans don't really have signs, to here, I'm just always using '+' if the imaginary component is nan. --- Signed-off-by: Brad Chamberlain <[email protected]>
--- Signed-off-by: Brad Chamberlain <[email protected]>
--- Signed-off-by: Brad Chamberlain <[email protected]>
--- Signed-off-by: Brad Chamberlain <[email protected]>
Also back out a previous change that forced the sign bit to be + in runtime conversions of complexes since we're deciding to print the sign bit now --- Signed-off-by: Brad Chamberlain <[email protected]>
The current plan for this effort is to enable the change using the proposed edition capability for breaking changes rather than a one-off flag, so this PR is on hold for the time being until someone has the opportunity to implement that. That said, there was also a decent amount of interest in having the feature and using this as a motivator to get it done. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this PR, if a complex had a nan value in either its real or imaginary component, the whole value would be stringified as simply
nan
rather thannan + nani
, say. This PR treats the two components separately w.r.t. file IO and string conversions, similar to how we treat complexes storinginf
and/or floating point values today. It also fixes a bug in string to complex conversions in which we were falling afoul of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48760 and addresses it using the same fix as in #24184The tests added here make sure that
nan
components in the real and/or imaginary slots are treated correctly for:complex
tostring
orbytes
string
orbytes
tocomplex
stdin
andstdout
)I also added a test of converting
inf
-oriented strings to real, complex, and imag that I wrote as background when I was researching this issue and updated a few existing tests whose output behavior improved as a result of this PR.Resolves #27007
TODO:
nan
values indicate the value of their sign bit, or always be represented as positive?