-
Notifications
You must be signed in to change notification settings - Fork 8
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
NXmpes refactoring #52
Conversation
@domna thanks for the summary, this is a very useful starting point to coordinate the discussion on NXmpes and related sub-methods. Below, I have added some comments (including the discussion during the workshop). Transformations
Alignment with ISO Standards
Experimental plansI fully agree that there should a separate description of the experimental plan somewhere in the application definiton (this is relevant for basically any application definition describing a physical measurement). For MPES, this should include the order of measurement, scan modes of the electronanalyzer (could be its own base class), scanning of external parameters, and so on. While it was discussed that it is probably not desirable to have NXmpes as the default software solution for these instruments (including automation of the devices), the measurement process and the order for operations should be reflected in the file as well, e.g. by something like NXprocedure (not existing yet). Multiple base classes in one nexus fileFor me, this is quite important going forward. While probably not originally intended by NIAC, it seems ot me that @domna's proposal (i.e., allowing for multiple bases classes in the Alignment with analysis softwareFor XPS, most of the relevant data analysis software solutions are listed here. Additionally, one could consider including Quases and COMPRO. The developers of these solutions should be included in the discussion at some point. Instrument
Calibration
Data quality and error detection
How to move forwardSince we already have quite a lot of discussion items here, I would suggest to have an internal meeting/workshop of some sort first to discuss all of these issues and come up with an enhanced proposal that we can then present to the wider community in a second step. |
@lukaspie @domna wrt to your last comment and topics some cents from my side: Transformations: ISO: Multiple base classes: Data quality etc. Instrument: Calibration: |
I think its good to work example based here and collected different examples from different communities. We already have an example from the mpes liquid community (by @stthuermer) and one for arpes (the matrices are there and currently @rettigl @Tommaso-Pincelli and I are trying to build an example to convert this into the pyarpes coordinate system. Our idea was to have different examples and try to compile something general from then. @lukaspie Do you have an transformation example for your xps experiment? I think this would be useful as well.
Yeah, I think we should try to buy a copy of this document.
I'm also thinking whether we can build guidelines for the data provenance along the analysis steps (e.g., pyarpes is keeping track in each analysis step). There is already something in this direction with the calibration data in NXprocess.
Fully agree. We might want to dedicate some days to discuss internally where we take our time to address each of these points individually. Maybe it would be good to have an in-person meeting for this, where we prepare and someone of us moderates through the different points based on the comments here, from the companies and the comments on the proposal page. (Edit: I will be, however, in holidays from thursday until 04.09. - so maybe we should discuss tomorrow in the TF meeting and maybe we can already set a date?) |
Here are some smaller points we need to address in NXmpes:
|
We want to have a meeting when everyone is back from holidays in mid september. For this there are some points for preparation we then can use in the discussion and which we should assign until then:
These are not strictly necessary for the discussion but could be very useful (especially the first point) to directly being able to document stuff during/after the meeting
Here are some points which are already worked on:
|
PR #53 should be merged in here, before we evaluate the base classes! |
How does this work together with NXtransformations? I.e. if I want to use this in an NXtransformation chain as a first element to base everything else on, how would I do this? |
38ed45e
to
937af09
Compare
Let's also add an NXpulser which can be used with an instance NXbeam |
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.
I found a number of things still to fix, see the review comments. It appears also that some base classes (in particular NXsample, NXenvironment, NXcalibration) are not up-to-date with their usage in NXmpes and our discussions.
I don't get what you mean here. |
Regarding that "some base classes (in particular NXsample, NXenvironment, NXcalibration) are not up-to-date with their usage in NXmpes and our discussions", I don't think that every field that is present in The only problem is really if there is an uppercase field in the base class (like |
In the original discussion about this, you suggested some changes, but they are not implemented yet, it seems. |
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.
Appart from a few comments made directly, this looks good now from my side.
I just realized that the generic |
Comments and changes from latest reviews by @mkuehbach and @rettigl are in. There are just two unresolved conversations left:
Associated PRs have also been merged it, aside from the sub add-defs and #144 (can all be merged later). |
This PR is dedicated to collect changes from the mpes workshop in may and to stimulate discussions on the base class changes introduced by NXmpes.
The comments from the collaborators can be found at a slightly older fairmat proposal page.
Associated PRs
Changes from mpes workshop #32 (Changes have been added here)NXmpes_liquid
#104Change NXsample in NXmpes using the new base classes, introduce NXactuator #68(merged)Remove sample history from MPES #82(closed)Add list of physical quantities as axis names to NXdata #92(merged)Use NXresolution in MPES #124(merged)Extend energy scan mode in NXenergydispersion #127(merged)Transformations
There was already a longer discussion on this during the design of the NXmpes_liquid in #27. The main question is how we denote actual geometries inside the transformations, i.e., should there be vectors which collect a certain axis of, e.g., a HEA.
Also there should be documentations (especially examples/how-tos/tutorials) how to build a transformation chain for template experiments. There is also the new documentation from the NIAC https://hdf5.gitlab-pages.esrf.fr/nexus/nxtransformations_active/classes/base_classes/NXtransformations.html which is much better at explaining how this transformations actually work. For visualization there is also https://github.com/domna/nexus3d.
There are also advances form @mkuehbach for representing coordinate systems in a cleaner way.
Another question to stimulate discussion: Should there be one central place to collect transformations or should they stay attached to the instruments?
Users
There are some comments that the user we have actually violates the GDPR and its questionable whether we can just collect this information.
Alignment with ISO Standards
During the workshop there were some discussions about already existing iso standards related to mpes (there are some comments in #32 https://www.iso.org/obp/ui/#iso:std:iso:18115:-1:ed-3:v1:en), i.e., standardised experimental names. There is also the PaNET ontology and nomad will probably start using KRISS (which is more related to organising physical quantities, e.g., from analysis. But it may be worth to take a look there, too).
Experimental plans
During the workshop there was the idea of an experimental plan, e.g., a description of the measurement routine which describes which parameters were scanned against what. It would give users a top-level approach to the data without having to search through various instruments (which are a rather specific approach to the metadata) and could be used in conjunction with NXdata or is a more sophisticated NXdata entry. I think somewhat related is a general structure of nexus files which should be the same throughout different appdefs and could also contain an experimental plan.
Multiple base classes in one nexus file
During the discussions for sub-appdefs we felt that it would be convenient in having a trait-like approach to select certain aspects of an experiment, e.g., having an NXmpes_timeresolved which can be used together in a file with an NXmpes_arpes to describe a timeresolve arpes experiment without creating a NXmpes_arpes_timeresolved sub-def. This could be simply denoted by an array in the
definition
field of NXentry using similar inheritance rules as traits (sometimes also known as interfaces) in programming languages, i.e., if there are duplicate fields the first array entry has precedence. We could also define appdefs with collisions incompatible or create a separate namespace for each definition. This can also be used for analysis, e.g., additional analysed fields can just be demanded + it can add interfacing for compatibility with other tools, e.g., we could add a NXnomad trait which builds the results in accordance to KRISS to denote searchable fields.Angle and k-resolved appdefs
For many companies it would be helpful to be able to write an angle based appdef directly and to have conversion routines to k-space based appdefs. Contrary, a TOF detector could write k-space definitions directly. How to properly represent the different aspects?
Alignment with sub-methods
Alignment with analysis software
We are currently benchmarking pyArpes (see https://github.com/FAIRmat-Experimental/arpes / https://github.com/chstan/arpes) as an analysis tool. It is unclear what its current maintenance status is (last commit may 2022). However, we should think about how we can map the appdef data into a general data structure as the one used by pyarpes, especially for angle-based appdefs.
Legacy data management and versioning
How do we deal which a history of instrument calibration? Do we only give the latest? Is there some sort of quality factor depending on how much time has passed since the last calibration.
Another point is versioning, in particular for the definitions. As the definitions are now a python package, we should try to extract the version from there (with setuptools-scm as we do for pynxtools). However, there are still some open questions, e.g., how do we deal with different version numbers between fairmat and niac? Do we just use distance + hash notation of setuptools-scm? Certainly, the repository should also be collected in the file. Do we want to have an official fairmat url which points to our nexus definitions repo? So we could in principle move it to somewhere else without breaking older files?
This is just what I can recall from the top of my head. Please feel free to extend and discuss here.
@Tommaso-Pincelli @rettigl @sanbrock @lukaspie @stthuermer