8000 Throw warning for variadic notation for non-variadic names by lukaspie · Pull Request #591 · FAIRmat-NFDI/pynxtools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Throw warning for variadic notation for non-variadic names #591

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
Closed
wants to merge 2 commits into from
Closed

Throw warning for variadic notation for non-variadic names #591

wants to merge 2 commits into from

Conversation

lukaspie
Copy link
Collaborator

No description provided.

@lukaspie lukaspie mentioned this pull request Mar 20, 2025
Merged
13 tasks
@lukaspie lukaspie force-pushed the update-defs-and-related-tools branch from 4bce7ac to 9ac4396 Compare March 20, 2025 11:30
@rettigl
Copy link
Collaborator
rettigl commented Mar 20, 2025

I generally support this, but maybe we only merge this once we have sibling inheritance properly solved?

@rettigl
Copy link
Collaborator
rettigl commented Mar 20, 2025

"/ENTRY/CALIBRATION[transmission_correction]"
This should not give a warning, no?

WARNING: Given group name 'CALIBRATION' conflicts with the non-variadic name 'transmission_correction (opt)', which should be of type NXcalibration.

@lukaspie
Copy link
Collaborator Author
lukaspie commented Mar 20, 2025 •
edited
Loading

I generally support this, but maybe we only merge this once we have sibling inheritance properly solved?

Yeah, that was the idea 👍 just wanted to have a starting point for this.

"/ENTRY/CALIBRATION[transmission_correction]"
This should not give a warning, no?
WARNING: Given group name 'CALIBRATION' conflicts with the non-variadic name 'transmission_correction (opt)', which should be of type NXcalibration.

That is indeed weird. Will need to check what is going on here.

Base automatically changed from update-defs-and-related-tools to master March 20, 2025 16:50
@lukaspie lukaspie force-pushed the concept-name-for-nonvariadic branch from bad368d to 3579f43 Compare April 9, 2025 10:55
@lukaspie
Copy link
Collaborator Author
lukaspie commented Apr 9, 2025 •
edited
Loading

"/ENTRY/CALIBRATION[transmission_correction]" This should not give a warning, no?

WARNING: Given group name 'CALIBRATION' conflicts with the non-variadic name 'transmission_correction (opt)', which should be of type NXcalibration.

This should be allowed now, but it goes a bit against the logic we enforce elsewhere. Since transmission_correction is a non-variadic concept, there should ideally not be a concept name given. I have implemented now that you can give the concept names for a non-variadic group if the concept name matches the group's type (i.e., CALIBRATION matches for NXcalibration).

However, if your template has

"/ENTRY[my_entry]/CALIBRATION[transmission_correction]/<some-field>" (<some-field> being required)
"/ENTRY[my_entry]/transmission_correction/<another-field>"

we would still get a warning like "The data entry corresponding to ENTRY[my_entry]/transmission_correction/<some-field> is required". This is because the get_variations_of function for transmission_correction stops at the second key and doesn't consider the one with the concept name given. We could change get_variations_of, but still each variant gets checked separately. So you either need to use CALIBRATION[transmission_correction] OR transmission_correction for all subelements.

See also this test and the one above.

So, the question is, do we even allow this? May be confusing if you write the template like above and you get a warning even though each element in the template is technically correct.

The same will happen if you have (assuming sibiling inheritance works)

"/ENTRY[my_entry]/data/AXISNAME[energy]"
"/ENTRY[my_entry]/data/energy/@type"

This will tell you that the @type attribute is written for a non-existing energy field.

@rettigl
Copy link
Collaborator
rettigl commented Apr 9, 2025 •
edited
Loading

What should this do for fields with given concept name? I was expecting similar behavior, but off course the type cannot be checked.
For keys
"/ENTRY/identifierNAME[identifier_entry]": "@attrs:metadata/loader/scan_path",
"AXISNAME_indices[@_indices]": "@DaTa:.index",
I still get the warnings:
WARNING: Given field name 'identifierNAME' conflicts with the non-variadic name 'identifier_entry (opt)'
WARNING: Field /ENTRY[entry]/identifierNAME[identifier_entry] written without documentation.
WARNING: Given attribute name 'AXISNAME_indices' conflicts with the non-variadic name '@energy_indices (rec)'
WARNING: Attribute /ENTRY[entry]/data/AXISNAME_indices[@energy_indices] written without documentation.

In particular the last one is not so easy to solve if we want to keep the "*" notation, as we cannot define all potential axisname_indices, yet want to define a few one, so they should be addressable using the same notation to be able to use the "*" construct.

@rettigl
Copy link
Collaborator
rettigl commented Apr 9, 2025

So, the question is, do we even allow this? May be confusing if you write the template like above and you get a warning even though each element in the template is technically correct.

Off course it would be helpful if there was not this restriction in resolution, but I think if this is difficult to implement, we could keep this and just add a small note in the documentation about the conversion template formalism.

@lukaspie
Copy link
Collaborator Author
lukaspie commented Apr 9, 2025

What should this do for fields with given concept name? I was expecting similar behavior, but off course the type cannot be checked. For keys "/ENTRY/identifierNAME[identifier_entry]": "@attrs:metadata/loader/scan_path", "AXISNAME_indices[@__indices]": "@DaTa:_.index", I still get the warnings: WARNING: Given field name 'identifierNAME' conflicts with the non-variadic name 'identifier_entry (opt)' WARNING: Field /ENTRY[entry]/identifierNAME[identifier_entry] written without documentation. WARNING: Given attribute name 'AXISNAME_indices' conflicts with the non-variadic name '@energy_indices (rec)' WARNING: Attribute /ENTRY[entry]/data/AXISNAME_indices[@energy_indices] written without documentation.

In particular the last one is not so easy to solve if we want to keep the "" notation, as we cannot define all potential axisname_indices, yet want to define a few one, so they should be addressable using the same notation to be able to use the "" construct.

In the current version, NXentry/identifier_entry is indeed a non-variadic term, so this is still just the problem with sibling inheritance, right? I was planning to address this next. Same for @energy_indices and @AXISNAME_indices.

@lukaspie
Copy link
Collaborator Author

Closing here as all changes were brought to #621

@lukaspie lukaspie closed this Apr 24, 2025
@lukaspie lukaspie deleted the concept-name-for-nonvariadic branch June 30, 2025 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
discussion validation
Projects
None yet
Milestone
No milestone
Development

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