FAIRmat-NFDI / nexus_definitions

Definitions of the NeXus Standard File Structure and Contents
https://manual.nexusformat.org/
Other
5 stars 8 forks source link

Add relative resolution to NXresolution, clean up ISO standard in NXphotoemission #178

Closed lukaspie closed 4 months ago

lukaspie commented 4 months ago

This PR depends on #74. I made this its own PR so that we can review it better.

Resolves #164 and #166.

lukaspie commented 4 months ago

I wonder if we should split NXenergydispersion\pass_energy to pass_energy for hemispherics and drift_energy for TOFs. Currently, in NXenergydispersion\pass_energy we link to the pass energy definition in the ISO standard which is not the same as the TOF drift energy.

rettigl commented 4 months ago

I wonder if we should split NXenergydispersion\pass_energy to pass_energy for hemispherics and drift_energy for TOFs. Currently, in NXenergydispersion\pass_energy we link to the pass energy definition in the ISO standard which is not the same as the TOF drift energy.

If we want to be strict with the terms, we can do that. Is there something for drift energy in ISO?

lukaspie commented 4 months ago

One could consider also adding the relative_resolution_errors at NXphotoemission and NXelectronanalyzer, even though not necessary (not different unit).

Exactly, that's why I didn't add them, as that would have the same fields in the base class as in app def.

I wonder if we should split NXenergydispersion\pass_energy to pass_energy for hemispherics and drift_energy for TOFs. Currently, in NXenergydispersion\pass_energy we link to the pass energy definition in the ISO standard which is not the same as the TOF drift energy.

If we want to be strict with the terms, we can do that. Is there something for drift energy in ISO?

I did not find anything for drift energy, at least not in that particular ISO standard. I guess this was mostly developed with XPS and hemispherical analyzers in mind. I would still argue it makes sense to split since they are distinct concepts, right?

lukaspie commented 4 months ago

If we add drift_energy, then we need to make pass_energy in NXphotoemission recommended. Does that work for you?

domna commented 4 months ago

If we add drift_energy, then we need to make pass_energy in NXphotoemission recommended. Does that work for you?

~Or we use a choice element for it? https://manual.nexusformat.org/defs_intro.html#choice~

~We would need to add tool support for it, though (but we need to add this at some point anyways):~

Sorry, it seems NIAC has only intended this for groups. So I guess it's the only choice to make it recommended.

lukaspie commented 4 months ago

Sorry, it seems NIAC has only intended this for groups. So I guess it's the only choice to make it recommended.

Ok, makes sense. So both pass_energy and drift_energy recommended or just pass_energy?

domna commented 4 months ago

Sorry, it seems NIAC has only intended this for groups. So I guess it's the only choice to make it recommended.

Ok, makes sense. So both pass_energy and drift_energy recommended or just pass_energy?

I would say both recommended with a note in the docstring that either one of them should be supplied, i.e., that their recommended for the respective technique.

lukaspie commented 4 months ago

Sounds good, I changed it to

pass_energy(NX_FLOAT):
  exists: recommended
  doc: |
    Either `pass_energy` or `drift_energy` must be supplied. `pass_energy` should be used when working
    with hemispherical analysers.
  unit: NX_ENERGY
drift_energy(NX_FLOAT):
  exists: recommended
  doc: |
    Either `pass_energy` or `drift_energy` must be supplied. `drift_energy` should be used if a TOF is used in the
    energy dispersive part of the electron analyzer.
  unit: NX_ENERGY
lukaspie commented 4 months ago

I have reverted the removal of NXmpes in this PR since we will have a discussion about naming the appdefs in the hierarchy again.