Skip to content

MPES: new concepts from NIAC discussions, searchable fields - #329

Merged
lukaspie merged 52 commits into
fairmatfrom
mpes-refactor
Mar 6, 2025
Merged

MPES: new concepts from NIAC discussions, searchable fields#329
lukaspie merged 52 commits into
fairmatfrom
mpes-refactor

Conversation

@lukaspie

@lukaspie lukaspie commented Feb 6, 2025

Copy link
Copy Markdown
Collaborator
  • Explicitly name the following axes in NXmpes/NXentry/data(NXdata):

    • energy
    • photon energy
    • kx, ky, kz
    • k_parallel, k_perpendicular
    • angular0, angular1
    • spatial0,spatial1
    • delay
    • temperature
  • Explicitly name the following axes in NXmpes/NXinstrument/NXelectronanalyser/NXdetector/raw_data(NXdata):

    • pixel_x, pixel_y
    • photon energy
    • kx, ky, kz
    • k_parallel, k_perpendicular
    • angular0, angular1
    • spatial0,spatial1
    • delay
    • temperature
  • NXprocess and subclasses are stored directly under Nxmpes/NXentry

    • AXIS_axis_calibration for general calibration on all axes that are defined in NXdata
    • Brought in special calibrations from existing base class NXprocess_mpes
  • As a consequence, remove base classes NXdata_mpes , NXdata_mpes_detector, and NXprocess_mpes

  • Allow for multiple beams and sources

    • beam_probe/source_probe
    • beam_pump/source_probe
    • beamTYPE/sourceTYPE -> I still think this is valuable, see the description in its docs
  • change all instances of NXidentifier to fields inheriting from identifierNAME

  • remove NXsubstance, use existing concepts in NXsample for now

    • use NXsample/chemical_formula
  • add field core_levels under ENTRY

    • list like NXdata/@axes
    • suggested enumeration of all possible core levels
  • make all NXtransformations/@vector attributes NX_NUMBER

  • add explicit open enumeration for function_type for the peak and background in NXxps/NXfit

Open questions:

  • Do we need k_parallel, k_perpendicular?
  • Is AXIS_axis_calibration good enough or do we need individual named calibrations for each axis?
  • Can we improve core_level description?

@rettigl rettigl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

_indices are INT

Comment thread contributed_definitions/NXmpes_arpes.nxdl.xml Outdated
Comment thread contributed_definitions/NXmpes.nxdl.xml Outdated
@rettigl

rettigl commented Feb 28, 2025

Copy link
Copy Markdown
Collaborator

@rettigl do we still need this comment?

Don't think so.

@lukaspie

Copy link
Copy Markdown
Collaborator Author

I remember you saying NXcoordinate_system_set should be renamed or replaced?

Correct, we will just not use it anymore (similar to having no grouping for Nxprocess). The coordinate systems are now directly under NXentry.

@lukaspie

Copy link
Copy Markdown
Collaborator Author

Is there a convention regarding the use of en-US vs. en-GB in Nexus? Our definitions are somewhat inconsistent here, e.g. "NXelectronanalyser" vs. "analyzer" at several places in the docstrings. I would suggest fixing this to one convention. Another example is "oxidising" vs. oxidizing.

There is not really an official guideline. Most of the documentation uses American English, but there are some other cases as well. Maybe @sanbrock has an idea?

@lukaspie
lukaspie marked this pull request as ready for review February 28, 2025 17:23
@lukaspie

lukaspie commented Mar 5, 2025

Copy link
Copy Markdown
Collaborator Author

Is there a convention regarding the use of en-US vs. en-GB in Nexus? Our definitions are somewhat inconsistent here, e.g. "NXelectronanalyser" vs. "analyzer" at several places in the docstrings. I would suggest fixing this to one convention. Another example is "oxidising" vs. oxidizing.

I went through the existing definitions in the NIAC repo, it seems like in almost all cases, the en-US version is used (e.g. optimization, minimization, etc.). It also makse sense since most NIAC members have been from the US and only a few of them from the UK.

If you agree, I would change all of our definiitions to be en-US, as well.

@rettigl

rettigl commented Mar 5, 2025

Copy link
Copy Markdown
Collaborator

If you agree, I would change all of our definiitions to be en-US, as well.

Yes, please.

@lukaspie

lukaspie commented Mar 5, 2025

Copy link
Copy Markdown
Collaborator Author

If you agree, I would change all of our definiitions to be en-US, as well.

Yes, please.

Done, see the last two commits. Main difference was analyser -> analyzer. Our tools will have to adjust to that as well.

As an addition, I also removed language like "NXenergydispersion is a sublass of NXelectronanalyzer", since it is not actually a subclass (in the inheritance sense), it is just used within.

If you don't have anything else to change @rettigl, I would merge here and bring the NIAC PR uptodate as well.

Comment thread contributed_definitions/NXfit.nxdl.xml

@rettigl rettigl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I now checked only the yamls and only the changes shown here, but they appear fine to me apart from a few comments.
I would suggest to implement necessary changes and update the examples meanwhile NIAC reviews this, so we can potentially identify problems by our examples.

Comment thread contributed_definitions/nyaml/NXenergydispersion.yaml
Comment thread contributed_definitions/nyaml/NXfit_function.yaml
Comment thread contributed_definitions/nyaml/NXmpes.yaml Outdated
Comment thread contributed_definitions/nyaml/NXmpes.yaml
Comment thread contributed_definitions/nyaml/NXmpes.yaml Outdated
Comment thread contributed_definitions/nyaml/NXmpes.yaml Outdated
Comment thread contributed_definitions/nyaml/NXmpes.yaml Outdated
@rettigl

rettigl commented Mar 5, 2025

Copy link
Copy Markdown
Collaborator

There is still a typo "presense" in NXbeam.yaml, line 17. We could propose to fix it in this PR or elsewhere.

@rettigl

rettigl commented Mar 5, 2025

Copy link
Copy Markdown
Collaborator

Similar US-EN "serlialization" instead of "serialisation" in NXcalibration.yaml, line 29

@rettigl

rettigl commented Mar 6, 2025

Copy link
Copy Markdown
Collaborator

Suggestion for further changes in NXcalibration:
I suggest renaming L74: coefficients(NXparameter) to calibration_parameters(NXparameters)
This contains variadic TERM, which can be used for any kind of parameter entering the calibration. I suggest to remove the notion of polynomial function, and just state that term names can be used in the fit function. scaling_factor and offset I would put as special cases also into this container. What do you think?

@lukaspie

lukaspie commented Mar 6, 2025

Copy link
Copy Markdown
Collaborator Author

Suggestion for further changes in NXcalibration: I suggest renaming L74: coefficients(NXparameter) to calibration_parameters(NXparameters) This contains variadic TERM, which can be used for any kind of parameter entering the calibration. I suggest to remove the notion of polynomial function, and just state that term names can be used in the fit function. scaling_factor and offset I would put as special cases also into this container. What do you think?

That is tricky because NXcalibration is now accepted already. We would need to deprecate coefficients and add a new group calibration_parameters(NXparameters). Maybe this could still be done as a clean-up before the new release though. I made an issue and will talk to Sandor about this a bit.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants