Skip to content
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

Move NXlens_em to base classes #1519

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

lukaspie
Copy link
Contributor

@lukaspie lukaspie commented Dec 9, 2024

This implements what was discussed in #1407 (comment). It moves NXlens_em from the contributed definitions to the base classes and adds it to NXsource as an renameable group. The difference between the NXlens_em from the contributed definitions and in base classes comes down to formatting, but no new terms have been added.

Note that NXlens_em itself is an extension of NXcomponent (which is meant as a basis for describing any components of an instrument). NXcomponent is also making use of NXcircuit (used for any sort of electronic circuit devices), so this is brought in here as well. These were originally to be discussed in #1424, but I think it's best to bring them in here.

domna and others added 26 commits December 9, 2024 17:29
… version of yaml.

Removing unintensional comments

# Conflicts:
#	base_classes/NXbeam.nxdl.xml
#	base_classes/NXdetector.nxdl.xml
#	base_classes/NXentry.nxdl.xml
#	base_classes/NXinstrument.nxdl.xml
#	base_classes/NXprocess.nxdl.xml
#	base_classes/NXsample.nxdl.xml
#	base_classes/NXsource.nxdl.xml
#	contributed_definitions/NXcollectioncolumn.nxdl.xml
#	contributed_definitions/NXmpes.nxdl.xml
# Conflicts:
#	base_classes/NXdetector.nxdl.xml
#	base_classes/NXinstrument.nxdl.xml
#	base_classes/NXsensor.nxdl.xml
#	base_classes/nyaml/NXdetector.yaml
#	base_classes/nyaml/NXinstrument.yaml
#	base_classes/nyaml/NXsensor.yaml
#	base_classes/nyaml/NXsource.yaml
# Conflicts:
#	base_classes/NXroot.nxdl.xml
#	contributed_definitions/NXmpes.nxdl.xml
# Conflicts:
#	base_classes/NXdata.nxdl.xml
#	base_classes/nyaml/NXdata.yaml
#	contributed_definitions/NXactuator.nxdl.xml
#	contributed_definitions/NXcalibration.nxdl.xml
#	contributed_definitions/NXelectronanalyser.nxdl.xml
#	contributed_definitions/NXresolution.nxdl.xml
#	contributed_definitions/nyaml/NXactuator.yaml
#	contributed_definitions/nyaml/NXcalibration.yaml
#	contributed_definitions/nyaml/NXelectronanalyser.yaml
#	contributed_definitions/nyaml/NXresolution.yaml
#	contributed_definitions/nyaml/NXsource.yaml
# Conflicts:
#	base_classes/nyaml/NXsource.yaml
# Conflicts:
#	base_classes/nyaml/NXsource.yaml
# Conflicts:
#	base_classes/nyaml/NXsource.yaml
# Conflicts:
#	base_classes/nyaml/NXsource.yaml
# Conflicts:
#	base_classes/nyaml/NXsource.yaml
#	contributed_definitions/NXdeflector.nxdl.xml
#	contributed_definitions/nyaml/NXdeflector.yaml
# Conflicts:
#	base_classes/nyaml/NXsource.yaml
#	contributed_definitions/NXdeflector.nxdl.xml
#	contributed_definitions/nyaml/NXdeflector.yaml
…ersion

# Conflicts:
#	applications/NXarpes.nxdl.xml
#	applications/nyaml/NXarpes.yaml
#	base_classes/NXaperture.nxdl.xml
#	base_classes/NXbeam.nxdl.xml
#	base_classes/NXdata.nxdl.xml
#	base_classes/NXdetector.nxdl.xml
#	base_classes/NXentry.nxdl.xml
#	base_classes/NXenvironment.nxdl.xml
#	base_classes/NXinstrument.nxdl.xml
#	base_classes/NXmonochromator.nxdl.xml
#	base_classes/NXroot.nxdl.xml
#	base_classes/NXsample.nxdl.xml
#	base_classes/NXsample_component.nxdl.xml
#	base_classes/NXsensor.nxdl.xml
#	base_classes/NXsubentry.nxdl.xml
#	base_classes/NXtransformations.nxdl.xml
#	base_classes/NXuser.nxdl.xml
#	base_classes/nyaml/NXaperture.yaml
#	base_classes/nyaml/NXbeam.yaml
#	base_classes/nyaml/NXdata.yaml
#	base_classes/nyaml/NXentry.yaml
#	base_classes/nyaml/NXenvironment.yaml
#	base_classes/nyaml/NXmonochromator.yaml
#	base_classes/nyaml/NXprocess.yaml
#	base_classes/nyaml/NXroot.yaml
#	base_classes/nyaml/NXsample.yaml
#	base_classes/nyaml/NXsample_component.yaml
#	base_classes/nyaml/NXsensor.yaml
#	base_classes/nyaml/NXsource.yaml
#	base_classes/nyaml/NXsubentry.yaml
#	base_classes/nyaml/NXtransformations.yaml
#	base_classes/nyaml/NXuser.yaml
#	contributed_definitions/nyaml/NXdetector.yaml
#	contributed_definitions/nyaml/NXinstrument.yaml
Co-authored-by: Aaron S. Brewster <[email protected]>
@lukaspie lukaspie marked this pull request as ready for review December 11, 2024 12:49
@lukaspie lukaspie added the NIAC vote needed PR needs an approving vote from NIAC before merge label Dec 11, 2024
@phyy-nx
Copy link
Contributor

phyy-nx commented Dec 18, 2024

Comment from Telco:

  • NXcomponent seems like a big change. And that many base classes should derive from it. The NIAC requests a survey of base classes to identify those which should and those which should not derive from NXcomponent. Examples include NXdetector which should, and NXdata which should not.
  • As a corollary, adding NXcomponent as a new parent class to classes such as NXdetector, NXbeam, etc., would be a significant change, so move NXcomponent to a new PR, and make this one dependent on that new PR

Alternate proposal: instead of NXcomponent, add the necessary new fields to NXobject. This would imply that NXobject has NXfabrication and NXcircuit, but that's ok. Better to have more possibilities on NXobject than to add more base classes in general.

Reviewed in the NIAC: took a straw poll and it was almost evenly split between the above two options. This will need more discussion. @nexusformat/developers please chime in here! Thanks!

@lukaspie
Copy link
Contributor Author

Comment from Telco:

  • NXcomponent seems like a big change. And that many base classes should derive from it. The NIAC requests a survey of base classes to identify those which should and those which should not derive from NXcomponent. Examples include NXdetector which should, and NXdata which should not.
  • As a corollary, adding NXcomponent as a new parent class to classes such as NXdetector, NXbeam, etc., would be a significant change, so move NXcomponent to a new PR, and make this one dependent on that new PR

Alternate proposal: instead of NXcomponent, add the necessary new fields to NXobject. This would imply that NXobject has NXfabrication and NXcircuit, but that's ok. Better to have more possibilities on NXobject than to add more base classes in general.

Reviewed in the NIAC: took a straw poll and it was almost evenly split between the above two options. This will need more discussion. @nexusformat/developers please chime in here! Thanks!

#1525 implements option 1 here and adds a list of all classes for which it would likely make sense to extend NXcomponent. This is more meant as an exploration/survey rather than a votable proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed enhancement NIAC should review The NIAC should review/discuss NIAC vote needed PR needs an approving vote from NIAC before merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants