EM refactoring / EMv2 #136
Conversation
…documentation, adding first round of changes relevant for the implementation of the refactoring of the em-related readers in pynxtools
…age of more general base classes NXserialized, NXidentifier, and CG base classes, also incorporate feedback from discussions with Cameca and generally allow that NXapm can store different state of LEAP-based analyses, reorganizing constraints is another reason for NXapm refactoring, ii) make the currently paraprobe-specific base classes more generally applicable: an example currently NXapm_paraprobe_nanochem allowed to store only the specific way how delocalization based analyses work and what they mean and generate when using paraprobe, but ideally we would like to use a base class NXdelocalization also store delocalizations done with other tools (foremost) those by cameca to arrive at a suggestion for a description format whereby users can store serialized analysis results from atom probe on zenodo in a pre-harmonized way despite not having an appdef designed for it yet, iii) reduce redundancies and make descriptions like for the em refactoring more succinct
…n a minimum example, next steps i) apply to paraprobe_ranger, ii) refactor NXapm appdef, iii) update pynxtools, iv) discuss
…m_sim to enable a description of simulation and measurement the work on NXapm is not yet finished, NXapm_new is the constraint set that then will become NXapm, the current NXapm will be refactored to NXapm_base, then the refactoring of the NXapm is completed, refactoring of paraprobe-specific base classes postpone for the reason of too much uncertainty yet with the here made conceptual changes of rigorously relying on base class inheritance
… logged_against, and base classes for processing of results
…n NXapm can be removed
…heck remains, NXsimilarity_grouping to edit
…open issue: appdefs require redefinition of properties from inherited base classes violates single source principle
…s 0572bfa9df6f513325024185ed1f1c1901d0758c
…hough, if the NXDL contains only field defintion with attribute version, no docstrings for both and an enumeration make local succeeds, if you add docstrings for definition and version make local throws an NXDL logic error although these docstrings should be supported without problems
mkuehbach
changed the title
|
Before this PR can be merged #140 has to be merged in and nyaml2nxdl should be run again over all NXDL files to get consistent dates, thereafter remaining issues with make local have to be fixed. However, none of these limitations block the reviewing of the changes and conceptual edits in the classes of this PR. |
Fixes and modifications to classes NXapm_paraprobe_* classes
There was a problem hiding this comment.
LGTM, my comments on the subset of files that I reviewed were addressed properly.
I cannot say much about the overall validity of the approach taken here (and in the APM PR) as 1) I lack the domanin knowledge, and 2) this PR has way too many files for me to review accurately. I could appreciate smaller, iterative changes in the future where I could give more concrete comments.
EDIT: Merge conflicts and CI failure should obviously be addressed before merging.
…oes not yet know this exists
APM and paraprobe-toolbox-specific refactoring aka APM NeXus v2
…s in the catchen test
…gestions and reviews and feedback from community should target NXem first before thinking about offsprings
|
In the future offsprings for NXem should inherit from the NXem appdef i.e. NXem_offspring(NXem) |
Cleanly removed NXem_base class
There was a problem hiding this comment.
I did only a quick overview check and found some minor things to discuss first (batch file, harmonization of NXpeak).
I don't know if I can review this in detail as I miss apm specific domain knowledge.
If this is not to urgent I would wait until we have fixed the dimension (and comment?) issue in nyaml. Currently, our files in fairmat are still correct so I wouldn't willingly introduce wrong changes.
|
also after cleanup of nxdl files, nyaml representation can be removed from the repository |
|
Please not again this discussion about removing or not yaml files - especially not with a disfunctional converter - I do not support removing them. Remove them with the last commit that one commits prior to a merging of fairmat states into official NIAC branches that i agree. |
No description provided.