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.

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

Fairmat 2024: specialized base classes on top of NXprocess #1420

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

Fairmat 2024: specialized base classes on top of NXprocess #1420

wants to merge 17 commits into from

Conversation

lukaspie
Copy link
Contributor

@lukaspie lukaspie commented Sep 23, 2024 •
edited
Loading

Adds several new base classes that all extend NXprocess:

  • NXcalibration: for describing calibration of an instrument and/or a data set (e.g., an energy axis) -> moved from contributed to base_classes
  • NXdistortion: post-processing distortion corrections
  • NXregistration: image registration

@lukaspie lukaspie marked this pull request as draft September 23, 2024 16:46
@lukaspie lukaspie marked this pull request as ready for review September 23, 2024 17:28
@sanbrock sanbrock mentioned this pull request Sep 29, 2024
Open
@lukaspie lukaspie linked an issue Sep 29, 2024 that may be closed by this pull request
NXprocess #1454
Open
@phyy-nx
Copy link
Contributor

phyy-nx commented Sep 30, 2024

NXcalibration already reviewed
NXdistortion: make a proper subclass of NXprocess, check for duplicated fields and remove them
NXregistration: make a proper subclass of NXprocess, check for duplicated fields and remove them

Accepted by NIAC vote as part of NIAC2024

@lukaspie
Copy link
Contributor Author

lukaspie commented Oct 1, 2024

Dev notes to keep track:

  • NXdistortion: made a proper subclass of NXprocess, removed duplicated fields
  • NXregistration: made a proper subclass of NXprocess, removed duplicated fields
  • NXprocess: removed REGISTRATION, CALIBRATION, DISTORTION

depends on / blocked by:

ToDo:

@lukaspie lukaspie mentioned this pull request Oct 8, 2024
Open
@sanbrock sanbrock mentioned this pull request Nov 6, 2024
Open
@phyy-nx phyy-nx mentioned this pull request Dec 6, 2024
Open
@lukaspie lukaspie force-pushed the fairmat-2024-nxprocess branch from 9bc0a3a to 2fd6160 Compare December 6, 2024 12:03
@lukaspie lukaspie changed the title Fairmat 2024: add base classes to NXprocess Fairmat 2024: specialized base classes on top of NXprocess Dec 6, 2024
@lukaspie
Copy link
Contributor Author

lukaspie commented Dec 6, 2024

@phyy-nx @sanbrock as NXcalibration is now extending NXprocess, it is not used within NXprocess anymore. Therefore, we could remove it here and bring it in with #1419. In that case, this whole PR comes down to NXdistortion and NXregistration as sub-classes of NXprocess, which has already been voted for by NIAC. So if you agree, I can remove the changes to NXcalibration and then this PR should be ready to be finally reviewed. Note that the failing CI/CD also comes from NXcalibration, so that would be solved.

This was referenced Dec 6, 2024
Open
Open
@lukaspie lukaspie mentioned this pull request Dec 6, 2024
Open
@phyy-nx
Copy link
Contributor

phyy-nx commented Dec 7, 2024

@phyy-nx @sanbrock as NXcalibration is now extending NXprocess, it is not used within NXprocess anymore. Therefore, we could remove it here and bring it in with #1419. In that case, this whole PR comes down to NXdistortion and NXregistration as sub-classes of NXprocess, which has already been voted for by NIAC. So if you agree, I can remove the changes to NXcalibration and then this PR should be ready to be finally reviewed. Note that the failing CI/CD also comes from NXcalibration, so that would be solved.

Hm, the comment above says

NXcalibration already reviewed

Where was it reviewed? Also, #1419 has not been voted on but this has. Wouldn't keeping NXcalibration in this PR simplify things?

@lukaspie
Copy link
Contributor Author

lukaspie commented Dec 9, 2024

@phyy-nx @sanbrock as NXcalibration is now extending NXprocess, it is not used within NXprocess anymore. Therefore, we could remove it here and bring it in with #1419. In that case, this whole PR comes down to NXdistortion and NXregistration as sub-classes of NXprocess, which has already been voted for by NIAC. So if you agree, I can remove the changes to NXcalibration and then this PR should be ready to be finally reviewed. Note that the failing CI/CD also comes from NXcalibration, so that would be solved.

Hm, the comment above says

NXcalibration already reviewed

Where was it reviewed? Also, #1419 has not been voted on but this has. Wouldn't keeping NXcalibration in this PR simplify things?

I think #1419 was voted on here: #1452 (comment) or am I missing something? For me, it would be okay to bring it in here or in #1419, as you prefer. I just didn't want to have to update it in multiple places.

@lukaspie
Copy link
Contributor Author

lukaspie commented Dec 9, 2024

Update: I fixed the build for NXcalibration. On second though, it might actually make sense to merge NXcalibration with this PR since this is also a subclass for NXprocess. The commits are the same, so it does not really matter. In any case, any merging of NXcalibration is still blocked by #1486.

@phyy-nx
Copy link
Contributor

phyy-nx commented Dec 18, 2024

From Telco: subclass NXregistration from NXprocess, and remove NXcalibration (already part of #1419). Then since this has been voted on, it needs an approval and can be merged.

domna and others added 7 commits December 19, 2024 12:10
@domna @lukaspie
Readds base classes for mpes (from commit 843283a)
6bf063b 
# Conflicts:
#	base_classes/NXaperture.nxdl.xml
@RubelMozumder @lukaspie
Regeneration of the nexus file for fixing the changes coming from old…
b73cd9d 
… 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
@lukaspie
format base classes and applications in accordance to existing NIAC v…
b5f8750 
…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/NXsource.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/NXdetector.yaml
#	base_classes/nyaml/NXentry.yaml
#	base_classes/nyaml/NXenvironment.yaml
#	base_classes/nyaml/NXinstrument.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
@lukaspie
revert unintentional changes from cherry-pick
550138f 
fix copyright dates
@lukaspie
pull out base classes cited in NXprocess
656049d
@lukaspie
move base classes to base_classes folder
29d99ea
@lukaspie
move NXfabrication to base_classes
6b18a55
@lukaspie lukaspie force-pushed the fairmat-2024-nxprocess branch from 98049c3 to 44f2450 Compare December 19, 2024 11:10
@lukaspie
Copy link
Contributor Author

From Telco: subclass NXregistration from NXprocess, and remove NXcalibration (already part of #1419). Then since this has been voted on, it needs an approval and can be merged.

@phyy-nx: NXregistration is now subclassed from NXprocess, and I removed NXcalibration. Should be ready for final review and merge. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phyy-nx phyy-nx phyy-nx left review comments

@prjemian prjemian prjemian left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement NIAC approved NIAC should review The NIAC should review/discuss
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NXprocess
5 participants
@lukaspie @phyy-nx @prjemian @domna @RubelMozumder