8000 add bool to NX_CHAR_OR_NUMBER by rettigl · Pull Request #590 · FAIRmat-NFDI/pynxtools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

add bool to NX_CHAR_OR_NUMBER #590

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 21, 2025
Merged

add bool to NX_CHAR_OR_NUMBER #590

merged 1 commit into from
Mar 21, 2025

Conversation

rettigl
Copy link
Collaborator
@rettigl rettigl commented Mar 19, 2025

Nexus states "Any valid character string or NeXus number representation", and thus I think it makes sense to also allow bool here.

@rettigl rettigl mentioned this pull request Mar 19, 2025
Merged
@lukaspie
Copy link
Collaborator

I am not sure this is correct, since neither NX_CHAR nor NX_NUMBER only booleans. Then why would their union support it?

@rettigl
Copy link
Collaborator Author
rettigl commented Mar 20, 2025

My reasoning was that currently, there is no way in nexus that a field can accept any kind of data. I would expect that NX_CHAR_OR_NUMBER should be precisely that.

@rettigl
Copy link
Collaborator Author
rettigl commented Mar 20, 2025

I am not sure this is correct, since neither NX_CHAR nor NX_NUMBER only booleans. Then why would their union support it?

Technically, we could include booleans also in NX_INT, then they would be contained in NX_NUMBER

@lukaspie
Copy link
Collaborator

My reasoning was that currently, there is no way in nexus that a field can accept any kind of data. I would expect that NX_CHAR_OR_NUMBER should be precisely that.

nexusformat/definitions#1246 (comment) introduced NX_CHAR_OR_NUMBER as the union of NX_CHAR and NX_NUMBER. IMO, this does not include NX_BOOLEAN.

I see that it would be nice to be able to define a field can accept any kind of data. However, with the way the types are currently implemented, I don't think it is really possible. I think the best solution would actually be to be able to write unions of types, like type="NX_NUMBER|NX_CHAR" instead of inventing workarounds like NX_CHAR_OR_NUMBER. But this requires again a lengthy NIAC discussion.

The question is, what we do we do for now in pynxtools? Do we go with the literal interpretation of NX_CHAR_OR_NUMBER as it was defined or do we allow booleans as well to be more flexible? I think either one is fine, but other validators will of course make difference choices.

@rettigl
Copy link
Collaborator Author
rettigl commented Mar 20, 2025

We can keep it as it is, I'll close this. It was just for the conversion parameters and a bool flag there, but I can also use NXcollection there.

@rettigl rettigl closed this Mar 20, 2025
@rettigl
Copy link
Collaborator Author
rettigl commented Mar 20, 2025

@lukaspie Interestingly enough, bool is a subclass of int:

isinstance(True, int)
True

I am thus wondering why I got a warning in the first place, I will check that.

@rettigl
Copy link
Collaborator Author
rettigl commented Mar 20, 2025

This is a bit inconsistent:

print(isinstance(value.item(), int))
print(isinstance(np.bool_, int))
print(isinstance(np.bool_, np.integer))

True
False
False

@rettigl
Copy link
Collaborator Author
rettigl commented Mar 20, 2025

From https://joergdietrich.github.io/python-numpy-bool-types.html:

The bool_ type is not a subclass of the int_ type (the bool_ is not even a number type). This is different than Python’s default implementation of bool as a sub-class of int.

@rettigl rettigl reopened this Mar 20, 2025
@lukaspie
Copy link
Collaborator

From joergdietrich.github.io/python-numpy-bool-types.html:

The bool_ type is not a subclass of the int_ type (the bool_ is not even a number type). This is different than Python’s default implementation of bool as a sub-class of int.

If bool is a subclass of int in Python, I think it's fine if we allow it,

lukaspie
lukaspie approved these changes Mar 21, 2025
View reviewed changes
Copy link
Collaborator
@lukaspie lukaspie left a comment

Choose a reason for hiding this comment

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

LGTM, but linting needs to be fixed

@rettigl rettigl force-pushed the add_bool_to_nxcharornumber branch from 18e552d to 105cd0d Compare March 21, 2025 10:53
@rettigl rettigl merged commit ba56f46 into master Mar 21, 2025
13 of 17 checks passed
@rettigl rettigl deleted the add_bool_to_nxcharornumber branch March 21, 2025 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukaspie lukaspie lukaspie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rettigl @lukaspie
0