Ray's suggestions added to FIRmat-NFDI nexus_definitions PR #402 (how FAIRmat sees it)#403
Merged
Merged
Conversation
Reviewer's GuideThis PR integrates Ray’s feedback by standardizing naming for event and instrument group classes in both EM and APM definitions, extending NXcorrector_cs to include optical components and broader context, and refining documentation with new usage notes and attribute enhancements. Class diagram for standardized event and instrument group renaming (EM and APM)classDiagram
class NXem_event_data {
+start_time: NX_DATE_TIME
+end_time: NX_DATE_TIME
+identifier_sample: NX_CHAR
+instrument: NXem_instrument
+user: NXuser
+image: NXimage
+spectrum: NXspectrum
}
class NXem_instrument {
+name: NX_CHAR
+location: NX_CHAR
+type: NX_CHAR
+ebeam_column: NXebeam_column
+tilt1: NX_NUMBER
+tilt2: NX_NUMBER
+rotation: NX_NUMBER
+position: NX_NUMBER[3]
}
class NXapm_event_data {
+start_time: NX_DATE_TIME
+end_time: NX_DATE_TIME
+instrument: NXapm_instrument
}
class NXapm_instrument {
+type: NX_CHAR
+location: NX_CHAR
+flight_path: NX_FLOAT
+pulsing_device: NXcomponent
}
NXem_event_data --> NXem_instrument
NXapm_event_data --> NXapm_instrument
NXem_event_data --> NXuser
NXem_event_data --> NXimage
NXem_event_data --> NXspectrum
Class diagram for extended NXcorrector_cs and NXaberration usageclassDiagram
class NXcorrector_cs {
+applied: NX_BOOLEAN
+description: NX_CHAR
+alignment: NXprocess
+c_3: NXaberration
+c_5: NXaberration
+c_7: NXaberration
+c_9: NXaberration
+c_5_1: NXaberration
+c_5_2: NXaberration
+c_5_3: NXaberration
+c_5_4: NXaberration
+c_5_5: NXaberration
+c_5_6_a: NXaberration
+c_5_6_b: NXaberration
+electromagnetic_lens: NXelectromagnetic_lens
+optical_lens: NXoptical_lens
+aperture: NXaperture
+deflector: NXdeflector
}
class NXaberration {
+magnitude: NX_NUMBER
+order: NX_INT
+name: NX_CHAR
+alias: NX_CHAR
}
NXcorrector_cs --> NXaberration
NXcorrector_cs --> NXelectromagnetic_lens
NXcorrector_cs --> NXoptical_lens
NXcorrector_cs --> NXaperture
NXcorrector_cs --> NXdeflector
Class diagram for updated measurement groups referencing new instrument/event typesclassDiagram
class NXem_measurement {
+instrument: NXem_instrument
+eventID: NXem_event_data [*]
}
class NXapm_measurement {
+instrument: NXapm_instrument
+eventID: NXapm_event_data [*]
}
NXem_measurement --> NXem_instrument
NXem_measurement --> NXem_event_data
NXapm_measurement --> NXapm_instrument
NXapm_measurement --> NXapm_event_data
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @mkuehbach - I've reviewed your changes - here's some feedback:
- The renaming of core group types (e.g. NXinstrument_em → NXem_instrument, NXevent_data_em → NXem_event_data) will break backward compatibility—please provide an aliasing or migration strategy so existing files continue to validate.
- There’s a typo in the NXcorrector_cs docs around the :ref:
NXoptical_lensreference (missing backtick)—please correct the markup to ensure proper linking. - You’ve added a long_name attribute to the intensity field in NXem, but it’s not applied consistently across all definitions—please unify the inclusion of long_name on intensity in both NYAML and NXDL versions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The renaming of core group types (e.g. NXinstrument_em → NXem_instrument, NXevent_data_em → NXem_event_data) will break backward compatibility—please provide an aliasing or migration strategy so existing files continue to validate.
- There’s a typo in the NXcorrector_cs docs around the :ref:`NXoptical_lens` reference (missing backtick)—please correct the markup to ensure proper linking.
- You’ve added a long_name attribute to the intensity field in NXem, but it’s not applied consistently across all definitions—please unify the inclusion of long_name on intensity in both NYAML and NXDL versions.
## Individual Comments
### Comment 1
<location> `base_classes/nyaml/NXaberration.yaml:12` </location>
<code_context>
Table 7-2 of Ibid. publication (page 305ff) documents how to convert from the Nion to the CEOS definitions.
Conversion tables are also summarized by `Y. Liao <https://www.globalsino.com/EM/page3740.html>`_ an introduction.
+
+ The use of the base class is not restricted to electron microscopy but can also be useful for classical optics.
</doc>
<field name="magnitude" type="NX_NUMBER" units="NX_ANY">
</code_context>
<issue_to_address>
Broader applicability is now mentioned, but the sentence could be more precise.
Please clarify the specific use cases for this class in classical optics to make the documentation more helpful.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
lukaspie
approved these changes
Jul 29, 2025
lukaspie
left a comment
lukaspie
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, but I haven't checked that all occurrences have been changed
|
@mkuehbach, thanks for such a quick response, and sorry to raise issues at the last minute. I have added my vote to approve nexusformat#1423. |
mkuehbach
changed the title
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This implements the changes requested by Ray on NXem and NXapm renaming of classes that is discussed in #402 (which is the view for the reviewers for NXem) to the fairmat branch so we can use it from the next pynxtools release onwards.
Summary by Sourcery
Integrate Ray’s comments by renaming instrument and event data groups in NXem and NXapm, broadening NXcorrector_cs and NXaberration for classical optics, adding NXoptical_lens support, and refining documentation and attributes for consistency.
New Features:
Enhancements: