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

Nxsample refactoring #56

Merged
merged 22 commits into from
Aug 31, 2023
Merged

Nxsample refactoring #56

merged 22 commits into from
Aug 31, 2023

Conversation

lukaspie
Copy link
Collaborator

Initial draft for major refactoring of NXsample.

NXsample can now hold several base classes that split the sample description into

  1. description of its components down to the chemical substance level -> NXsample_component, NXsample_component_set, NXsubstance, NXsample_surface_dopant as an example for an NXsubstance
  2. any physical process that was performed prior/during the experiment -> NXphysical_process, NXsample_synthesis_step, ... -> can also be stored in NXsample_history
  3. a description of the sample history -> NXsample_history, made up of one or more NXphysical_process instances
  4. sample environment -> for now still allowed as NXenvironment in NXsample, but should eventually be on the level of the application definitions (still needs to be discussed)

In general, most of the new base classes in contributed_definitions don't contain much information yet and will be fleshed out in the future. This process will also heavily involve input from Area A's base sections, especially for description of physical processes and substances/sample components.

There are also new base classes for descriptions of single crystals (NXsingle_crystal) and related unit cell (NXunit_cell), based on what was available already in the old NXsample. So far, they are not in use anywhere, but one could imagine that in the future, they may be part of a larger ecosystem describing the shape and type of the sample (components), including other types of samples like liquids, powders, pellets, ...

For better discussion, the new/changed nyaml files have also been added. This also includes a NXsample_without_deprecated.yaml file that show the new NXsample without the many deprecated fields. They should obviously be cleared later on.

NXsample can now hold several base classes that split the sample description into
1) description of its components down to the chemical substance level -> NXsample_component, NXsample_component_set, NXsubstance, NXsample_surface_dopant as an example for an NXsubstance
2) any physical process that was performed prior/during the experiment -> NXphysical_process, NXsample_synthesis_step, ... -> can also be stored in NXsample_history
3) a description of the sample history -> NXsample_history, made up of one or more NXphysical_process instances
4) sample environment -> for now still allowed as NXenvironment in NXsample, but should eventually be on the level of the application definitions
This also contains a NXsample_without_deprecated.yaml file that show the new NXsample without the many deprecated fields.
- Fix small error in intendation in NXsingle_crystal.yaml
- Create nxdl once again
There a </field> missing
@mkuehbach mkuehbach requested a review from budschi August 24, 2023 11:54
@mkuehbach
Copy link
Collaborator

Hi @lukaspie, thank you very much for pushing this.
I will be leaving now all my comments on the yaml files for convenience and clarity.

Copy link
Collaborator

@mkuehbach mkuehbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukaspie finally what we needed!
Please find my comments below, happy to exchange tomorrow how I can most productively support you with

contributed_definitions/nyaml/NXunit_cell.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXunit_cell.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXunit_cell.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXunit_cell.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXunit_cell.yaml Outdated Show resolved Hide resolved
base_classes/nyaml/NXsample_component.yaml Show resolved Hide resolved
base_classes/nyaml/NXsample_component.yaml Show resolved Hide resolved
base_classes/nyaml/NXsample_component.yaml Show resolved Hide resolved
base_classes/nyaml/NXsample_component.yaml Show resolved Hide resolved
base_classes/nyaml/NXsample_component.yaml Show resolved Hide resolved
@mkuehbach
Copy link
Collaborator

mkuehbach commented Aug 24, 2023

For all to support you with finding some of the base class suggestions, I have made in the comments above, I am talking about nyaml files in the contributed/nyaml of the following PR #51

Copy link
Collaborator

@rettigl rettigl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good start. What I don't quite understand is the intended use/hierarchy of the Nx_sample_component/NX_physical_process subclasses. As far as I understand they are not cited in the base class hierarchy.
Does Nexus Inheritance also work in the sense that you can substitute a cited base class with an instance of a child class of that base class? I always thought the inheritance only goes as far as it includes all the super class elements.

base_classes/nyaml/NXsample.yaml Show resolved Hide resolved
base_classes/nyaml/NXsample.yaml Outdated Show resolved Hide resolved
base_classes/nyaml/NXsample.yaml Outdated Show resolved Hide resolved
base_classes/nyaml/NXsample_component.yaml Show resolved Hide resolved
base_classes/nyaml/NXsample_component.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXsample_substrate.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXsample_surface_dopant.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXsingle_crystal.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXsingle_crystal.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXsingle_crystal.yaml Outdated Show resolved Hide resolved
Change base classes:
- NXsample_component_set
- NXsubstance
- NXunit_cell
New(changed base classes:
- NXsample
- NXsample_component
- NXsample_history
- NXactivtiy
- NXphysical_process
- NXsample_synthesis_step
@lukaspie
Copy link
Collaborator Author

In the most recent commits, I have distilled the changes to simply adding the new base classes on NXsample and Nxsample_component, without deprecating anything (as @sanbrock suggested).

- Update some links/terms
- Remove non-supported base class inheritance wrt. NXactivity
All activity-related base classes now contain a NXnote for any data or other descriptors acquired during the activity.
These will be addressed by a new PR.
There can only ever be one NXsample_component_set in any NXsample or NXsample_component.
There can only ever be one NXsubstance in any NXsample or NXsample_component -> in this case, we are talking about a pure material
Describe the dependancy of the sample_ component to an instance of the NXsample_component_set (at the same level of granularity where the instance of  component is located).
@lukaspie
Copy link
Collaborator Author

lukaspie commented Aug 29, 2023

Any description of single crystals and unit cells has been removed for now and will be addressed in the branch crystal_structure_refactor.

In order to know the ordering of components in the concentaration, volume_Fraction, ... fields, we need an array of strings refering to the names of the NXsample_components. The order of these components serves as an index (starting at 1).
@lukaspie lukaspie marked this pull request as ready for review August 30, 2023 11:20
base_classes/nyaml/NXsample.yaml Outdated Show resolved Hide resolved
base_classes/nyaml/NXsample.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXactivity.yaml Show resolved Hide resolved
Copy link

@sanbrock sanbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a single question on sample and/or component vs.component_set

base_classes/nyaml/NXsample_component.yaml Show resolved Hide resolved
Copy link

@sanbrock sanbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sanbrock sanbrock dismissed mkuehbach’s stale review August 31, 2023 10:06

it has been discussed

@sanbrock sanbrock merged commit 4a37245 into fairmat Aug 31, 2023
4 checks passed
@lukaspie lukaspie deleted the NXsample_refactoring branch September 19, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants