-
Notifications
You must be signed in to change notification settings - Fork 9
Preparation for SPM proposal goes to the NIAC contributed_definitions (NXDL 2025) #404
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Lukas Pielsticker <[email protected]> Co-authored-by: markus.kuehbach <[email protected]> Co-authored-by: Markus Kühbach <[email protected]> Co-authored-by: RubelMozumder <[email protected]> Co-authored-by: Rubel <[email protected]>
Reviewer's GuideThis PR imports a comprehensive set of NeXus XML definitions for the SPM proposal by copying files from the CleanSpm branch. It establishes a core NXspm application, specialized extensions for AFM/STM/STS, and a suite of supporting base classes for bias spectroscopy, lock-in amplifiers, cantilevers, scan region/pattern/control, piezo configuration/material, and positioner. ER diagram for new SPM NeXus application and base typeserDiagram
NXspm ||--o{ NXafm : extends
NXspm ||--o{ NXstm : extends
NXspm ||--o{ NXsts : extends
NXspm ||--o{ NXspm_scan_control : has
NXspm ||--o{ NXspm_scan_region : has
NXspm ||--o{ NXspm_scan_pattern : has
NXspm ||--o{ NXspm_piezo_config : has
NXspm ||--o{ NXspm_piezoelectric_material : has
NXspm ||--o{ NXspm_positioner : has
NXspm ||--o{ NXspm_cantilever : has
NXspm ||--o{ NXspm_lockin : has
NXspm ||--o{ NXspm_bias_spectroscopy : has
NXspm_scan_control ||--o{ NXspm_scan_region : has
NXspm_scan_control ||--o{ NXspm_scan_pattern : has
NXspm_scan_pattern ||--o{ NXdata : has
NXspm_piezo_config ||--o{ NXspm_piezoelectric_material : has
NXspm_piezo_config ||--o{ NXcalibration : has
NXspm_positioner ||--o{ NXpid_controller : has
NXspm_cantilever ||--o{ NXcomponent : has
NXspm_cantilever ||--o{ NXobject : has
NXspm_lockin ||--o{ NXfabrication : has
NXspm_bias_spectroscopy ||--o{ NXspm_positioner : has
NXspm_bias_spectroscopy ||--o{ NXcircuit : has
NXspm_bias_spectroscopy ||--o{ NXspm_scan_control : has
Class diagram for new SPM NeXus definitions (NXspm and extensions)classDiagram
class NXspm {
<<application>>
+definition: NXspm
+experiment_technique: STM | STS | AFM
+scan_mode
+scan_type
+identifier_experiment
+NXprocess
+NXinstrument
+NXsample
+NXdata
+reproducibility_indicators
+resolution_indicators
}
class NXafm {
<<application>>
+definition: NXafm
+scan_mode
+NXinstrument
+reproducibility_indicators
+resolution_indicators
}
class NXstm {
<<application>>
+definition: NXstm
+scan_mode
+NXinstrument
+reproducibility_indicators
+resolution_indicators
}
class NXsts {
<<application>>
+definition: NXsts
+scan_mode
+NXinstrument
+reproducibility_indicators
+resolution_indicators
}
NXafm --|> NXspm
NXstm --|> NXspm
NXsts --|> NXspm
Class diagram for new SPM base classes (scan, piezo, cantilever, lock-in, etc.)classDiagram
class NXspm_scan_control {
<<base>>
+scan_time_start
+scan_time_end
+independent_scan_axes
+scan_resolutionN
+accuracyN
+scan_type
+scan_control_type
+scan_region: NXspm_scan_region
+mesh_SCAN: NXspm_scan_pattern
+spiral_SCAN: NXspm_scan_pattern
+snake_SCAN: NXspm_scan_pattern
+traj_SCAN: NXspm_scan_pattern
+linear_SCAN: NXspm_scan_pattern
}
class NXspm_scan_region {
<<base>>
+scan_offsetN
+scan_rangeN
+scan_angleN
+scan_startN
+scan_endN
}
class NXspm_scan_pattern {
<<base>>
+scan_speedN
+forward_speedN
+backward_speedN
+channelNAME
+scan_pointsN
+steppingN
+step_sizeN
+continuousN
+oscillating
+oscillation_frequency
+number_of_trajectory_points
+trajectory_points
+spiral_radiusN
+SCAN_DATA: NXdata
}
class NXspm_piezo_config {
<<base>>
+piezo_material: NXspm_piezoelectric_material
+calibration: NXcalibration
}
class NXspm_piezoelectric_material {
<<base>>
+name
+identifier_piezo_material
+chemical_description: NXsubstance
+type
+density
+relative_permittivity
+D_piezoelectric_constant
+G_voltage_constant
+K_electromechanical_constant
+P_pyroelectric_constant
+acoustic_impedance
+young_modulus
+surface_resistivity
+temperature_range
+glass_transition_temperature
}
class NXspm_positioner {
<<base>>
+z_controller: NXpid_controller
+z_offset
+controller_label
}
class NXspm_cantilever {
<<base>>
+cantilever_oscillator: NXcomponent
+cantilever_config: NXobject
}
class NXspm_lockin {
<<base>>
+hardware: NXfabrication
+bias_divider
+modulation_status
+modulation_signal
+lockin_current_flip_sign
+reference_amplitude
+reference_frequency
+reference_phase
+demodulated_signal
+demodulated_frequency
+frequency_demodulation_bandwidth
+phase_modulation_bandwidth
+amplitude_modulation_bandwidth
+demodulated_amplitude
+demodulated_phase
+demodulator_channels
+low_pass_N
+hi_pass_N
+lp_filter_order_N
+hp_filter_order_N
+ref_phase_N
+integration_time
+harmonic_order_N
+sensitivity_factor
}
class NXspm_bias_spectroscopy {
<<base>>
+measurement_type
+NXspm_positioner
+NXcircuit
+bias_sweep: NXspm_scan_control
}
NXspm_scan_control --> NXspm_scan_region
NXspm_scan_control --> NXspm_scan_pattern : mesh_SCAN, spiral_SCAN, etc.
NXspm_scan_pattern --> NXdata : SCAN_DATA
NXspm_piezo_config --> NXspm_piezoelectric_material
NXspm_piezo_config --> NXcalibration
NXspm_positioner --> NXpid_controller : z_controller
NXspm_cantilever --> NXcomponent : cantilever_oscillator
NXspm_cantilever --> NXobject : cantilever_config
NXspm_lockin --> NXfabrication : hardware
NXspm_bias_spectroscopy --> NXspm_positioner
NXspm_bias_spectroscopy --> NXcircuit
NXspm_bias_spectroscopy --> NXspm_scan_control : bias_sweep
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @RubelMozumder - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `contributed_definitions/NXspm.nxdl.xml:199` </location>
<code_context>
+ Note: group name (e.g. entry in ENTRY[entry]) inside the square bracket would be the `exact` name of the NXentry group.
+ </doc>
+ </group>
+ <group name="voltage_SENSOR" type="NXsensor" optional="true">
+ <doc>
+ This should be a link to the voltage_SENSOR under the instrument group:
</code_context>
<issue_to_address>
Inconsistent group naming: 'voltage_SENSOR' vs. 'voltage_sensorTAG'.
Standardize group naming conventions for voltage sensor groups to avoid confusion and potential schema or tooling errors.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
<group name="voltage_SENSOR" type="NXsensor" optional="true">
<doc>
This should be a link to the voltage_SENSOR under the instrument group:
ENTRY[entry]/INSTRUMENT[instrument]/voltage_SENSOR[*].
Note: group name (e.g. entry in ENTRY[entry]) inside the square bracket would be the `exact` name of the NXentry group.
</doc>
</group>
=======
<group name="voltage_sensor" type="NXsensor" optional="true">
<doc>
This should be a link to the voltage_sensor under the instrument group:
ENTRY[entry]/INSTRUMENT[instrument]/voltage_sensor[*].
Note: group name (e.g. entry in ENTRY[entry]) inside the square bracket would be the `exact` name of the NXentry group.
</doc>
</group>
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `contributed_definitions/NXspm.nxdl.xml:347` </location>
<code_context>
+ </field>
+ </group>
+ </group>
+ <group name="voltage_sensorTAG" type="NXsensor" nameType="partial" optional="true">
+ <doc>
+ The sensor information for the voltage device. Any voltage sensor involved in the
</code_context>
<issue_to_address>
Ambiguity in 'TAG' placeholder for group name.
Please clarify how 'TAG' should be replaced in 'voltage_sensorTAG' and provide explicit guidance or a formal naming pattern to ensure consistency.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
<group name="voltage_sensorTAG" type="NXsensor" nameType="partial" optional="true">
<doc>
The sensor information for the voltage device. Any voltage sensor involved in the
experiment or in any special measurement or in any specialized experiment component
can be registered under this group.
For this purpose, replace the TAG with the specific name or ID of the voltage sensor.
Do not register this group for sample bias voltage (DC), for that we have sample_bias_voltage group.
</doc>
=======
<group name="voltage_sensor{TAG}" type="NXsensor" nameType="partial" optional="true">
<doc>
The sensor information for the voltage device. Any voltage sensor involved in the
experiment or in any special measurement or in any specialized experiment component
can be registered under this group.
Replace the {TAG} placeholder in the group name with a unique, descriptive identifier for the voltage sensor, such as its model, location, or function (e.g., voltage_sensorPreamp, voltage_sensorSample, voltage_sensor1).
The naming pattern should be: voltage_sensor{SensorIdentifier}, where {SensorIdentifier} is a concise, alphanumeric string that uniquely identifies the sensor within the experiment context.
Do not register this group for sample bias voltage (DC); for that, use the sample_bias_voltage group.
</doc>
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `contributed_definitions/NXsts.nxdl.xml:116` </location>
<code_context>
+ Note: The group name written inside the square brackets (e.g. entry in ENTRY[entry]) would be the exact instance name of the base class (e.g. NXentry) in the nexus file.
+ </doc>
+ </field>
+ <field name="current_offset" type="NX_NUMBER" optional="true">
+ <doc>
+ The offset in tunneling current between tip and sample after application of bias voltage.
</code_context>
<issue_to_address>
current_offset field in reproducibility_indicators links to current instead of current_offset.
The docstring should reference 'current_sensor/current_offset' instead of 'current_sensor/current' to accurately reflect the field's purpose.
</issue_to_address>
### Comment 4
<location> `contributed_definitions/NXsts.nxdl.xml:144` </location>
<code_context>
+ Note: The group name written inside the square brackets (e.g. entry in ENTRY[entry]) would be the exact instance name of the base class (e.g. NXentry) in the nexus file.
+ </doc>
+ </group>
+ <field name="modulation_signal_type" optional="true">
+ <doc>
+ This is the signal on which the modulation voltage or current will be added.
</code_context>
<issue_to_address>
modulation_signal_type field is referenced but not defined in lockin_amplifier.
'modulation_signal_type' is referenced, but only 'modulation_signal' is defined in lockin_amplifier. Please align the field names or clarify their relationship to avoid confusion.
</issue_to_address>
### Comment 5
<location> `contributed_definitions/NXstm.nxdl.xml:198` </location>
<code_context>
+ Note: The group name written inside the square brackets (e.g. entry in ENTRY[entry]) would be the exact instance name of the base class (e.g. NXentry) in the nexus file.
+ </doc>
+ </group>
+ <field name="modulation_signal_type" optional="true">
+ <doc>
+ This is the signal on which the modulation voltage or current will be added.
</code_context>
<issue_to_address>
modulation_signal_type field is referenced but not defined in lockin_amplifier.
'modulation_signal_type' is used here, but only 'modulation_signal' is defined in lockin_amplifier. Please align the field names or document their relationship to avoid confusion.
</issue_to_address>
### Comment 6
<location> `contributed_definitions/NXspm_piezo_config.nxdl.xml:34` </location>
<code_context>
+ is applied on the material and it deforms proportional to the applied voltage.
+ Description below shows calibration coefficients and other configuration parameters of open loop piezo actuators (that is actuators without capacitive sensor feedback systems).
+ </doc>
+ <group name="piezo_material" type="NXspm_piezoelectric_material">
+ <doc>
+ The material description and properties of the piezoelectric scanner materials.
</code_context>
<issue_to_address>
piezo_material group is required, which may not always be available.
Consider marking 'piezo_material' as optional to accommodate cases where material details are unavailable.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
<group name="piezo_material" type="NXspm_piezoelectric_material">
=======
<group name="piezo_material" type="NXspm_piezoelectric_material" minOccurs="0">
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| Note: The group name written inside the square brackets (e.g. entry in ENTRY[entry]) would be the exact instance name of the base class (e.g. NXentry) in the nexus file. | ||
| </doc> | ||
| </field> | ||
| <field name="current_offset" type="NX_NUMBER" optional="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: current_offset field in reproducibility_indicators links to current instead of current_offset.
The docstring should reference 'current_sensor/current_offset' instead of 'current_sensor/current' to accurately reflect the field's purpose.
| Note: The group name written inside the square brackets (e.g. entry in ENTRY[entry]) would be the exact instance name of the base class (e.g. NXentry) in the nexus file. | ||
| </doc> | ||
| </group> | ||
| <field name="modulation_signal_type" optional="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: modulation_signal_type field is referenced but not defined in lockin_amplifier.
'modulation_signal_type' is referenced, but only 'modulation_signal' is defined in lockin_amplifier. Please align the field names or clarify their relationship to avoid confusion.
| Note: The group name written inside the square brackets (e.g. entry in ENTRY[entry]) would be the exact instance name of the base class (e.g. NXentry) in the nexus file. | ||
| </doc> | ||
| </group> | ||
| <field name="modulation_signal_type" optional="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: modulation_signal_type field is referenced but not defined in lockin_amplifier.
'modulation_signal_type' is used here, but only 'modulation_signal' is defined in lockin_amplifier. Please align the field names or document their relationship to avoid confusion.
| is applied on the material and it deforms proportional to the applied voltage. | ||
| Description below shows calibration coefficients and other configuration parameters of open loop piezo actuators (that is actuators without capacitive sensor feedback systems). | ||
| </doc> | ||
| <group name="piezo_material" type="NXspm_piezoelectric_material"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: piezo_material group is required, which may not always be available.
Consider marking 'piezo_material' as optional to accommodate cases where material details are unavailable.
| <group name="piezo_material" type="NXspm_piezoelectric_material"> | |
| <group name="piezo_material" type="NXspm_piezoelectric_material" minOccurs="0"> |
Co-authored-by: domna <[email protected]> Co-authored-by: Florian Dobener <[email protected]> Co-authored-by: kuehbachm <[email protected]> Co-authored-by: Lukas Pielsticker <[email protected]> Co-authored-by: markus.kuehbach <[email protected]> Co-authored-by: Markus Kühbach <[email protected]> Co-authored-by: mkuehbach <[email protected]> Co-authored-by: Pete R Jemian <[email protected]> Co-authored-by: RubelMozumder <[email protected]> Co-authored-by: RubelMozumder <[email protected]> Co-authored-by: Rubel <[email protected]>
Co-authored-by: Florian Dobener <[email protected]> Co-authored-by: Lukas Pielsticker <[email protected]> Co-authored-by: markus.kuehbach <[email protected]> Co-authored-by: Pete R Jemian <[email protected]> Co-authored-by: RubelMozumder <[email protected]> Co-authored-by: Sherjeel Shabih <[email protected]>
fe60b45 to
650dd99
Compare
FAIRmat-NFDI's nexus_definitions
mainbranch is by agreement synced up always with NIAC's main branch.Files are copied from the branch CleanSpm available in 401.