-
Notifications
You must be signed in to change notification settings - Fork 143
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
complex number support #558
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need these global setter functions?
What's the rationale for being able to enable/disable complex support? Same with set_complex_field_names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case a better way to deal with such global constants is to use
Ref
's with appropriate setters.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented it this way following what is done by h5py and for compatibility with existing practice/workflows. Since there is no actual hdf5 standard for complex numbers allowing the fieldnames to be set dynamically lets you interface more easily with existing conventions. For example I think that octave uses real/imag and I have also seen re/im. Also it's conceivable that somebody is storing a compound type with these fieldnames that doesn't represent a complex number. I'm not sure if this flexibility is necessary/worth it but I decided to follow the h5py precedent in allowing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why they would want to do that, but even if they did couldn't they just change the field names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kleinhenz that makes sense
Can we please update all the global constants to use refs (it's precompile friendly, as opposed to globals)
And update everywhere else to use
COMPLEX_SUPPORT[]
instead ofcomplex_support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the rational of being able to disable this feature. It allows to to read the complex numbers in its "raw" representation, which otherwise would not be possible. It also does not really hurt making this optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at
h5py
they don't offer a seperate way to enable/disable complex support.I kinda agree with @benchislett that even if they would want to do that they could just change the field names.
I'm not 100% convinced which approach is best. I'm leaning towards no option to enable disable support since that seems a little overboard. OTOH we can always remove this non-user facing functions in the future if deemed overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are correct that h5py doesn't provide a mechanism for completely disabling complex support. I don't have a strong preference on this either. I lean towards including it just to be the least disruptive to existing workflows but I could see it the other way also.
One point in favor is that changing the field names will prevent you from reading data as complex when you don't want to but it can't prevent you from accidentally writing complex data in an unwanted format where previously it would have thrown an exception so I think including the enable/disable thing is somewhat safer for people with a preexisting way of handling complex data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok that's convincing enough 👍