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

Base class templates #51

Merged
merged 50 commits into from
Nov 30, 2023
Merged

Base class templates #51

merged 50 commits into from
Nov 30, 2023

Conversation

mkuehbach
Copy link
Collaborator

@mkuehbach mkuehbach commented Aug 9, 2023

Some ideas how to make our NeXus concepts cleaner and easier to edit, inspect the first commit on this base_class_templates branch for further details. This PR is just a draft to help channel the discussion and feedback,
I have not implemented yet the changes in nyaml2nxdl but from what I have seen this seems to be technically not so much the challenge much more relevant is to discuss how mathematically number sets like N, N0, R, R+, Z map to NX_ data types bidirectionally, to my understanding not all do, e.g. when I say R+ it means it is an NX_NUMBER but I constrain more a positive number but not zero. This is along the lines from the NeXus code camp "how to get more of the camel's nose into NeXus".
The short hand notation dim: and then numpy notation for tensors I think is much cleaner for almost all cases we have used so far.

I am sorry that this PR is currently polluted with changes introduced from the spm branch, because that spm branch has not been merged yet into fairmat. So please keep an eye on the nyaml directory in the contributed definitions to inspect and comment on my suggestions. e8018d1 is the key commit with the suggestions.

markus.kuehbach added 13 commits August 9, 2023 13:18
… primitives inherit from new base class NXcg_primitive_set which makes a substantial number of repeated properties of specialized primitive base classes unnecessary, ii) introducing a rigorous mathematical set theoretical approach to describe the number type N, N0, Z, R, R+, R, iii) introduce short hand notation to reduce writing costs for shape/dimensions of arrays: now numpy syntax is used, for scalars (), (1, can be omitted, experience in FAIRmat has shown that in almost all cases the optional doc string to a dimension was almost never used and thus yaml based descriptions can be written more succinct, iv) proof-read docstrings and shortened them, introduced the suggestion to use info: as a keyword which should for now be just appended to the docstring as usual but in the future should be treated as a comment, i.e. only meant for human contextualization but ignored for semantic interpretation of concepts, a key difference that I hope this will bring is to reduce also the length of base classes and appdef as they show up in html pages, specifically for appdefs like NXem many of the conceptual ideas which these docstrings currently contain would be much better placed in a proper documentation instead of the actual class definition in the hope that the overall appearance of classes becomes easier and faster for people to power read; maybe this also works against some criticism that NeXus is considered complex, I have to say with the outsourcing of NXcg_primitive_set I feel it is damn simple now
…ll not be refactored before the APT&M2023 conference in Sept2023, also because feedback from Leoben was positive enough that no immediate changes wrt to appdefs and base classes were required
…s with the proposed NXcoordinate_system and NXcoordinate_system_set base classes and best practice recommendations how they should be used including feedback from discussions with Florian Dobner and Tamás Haraszti
…ic conventions for EM and methods used in EM
…istribution function, event_data, stage_lab, and ebsd_crystal_structure_model
…specific deep base classes to describe the terms and taxononmy of method-specific branches to be hooked into appdefs like NXem
…chema set for electron microscopy research, two tasks remain: i) refactor what is now a method-specific instance rather than an appdef (NXem_ebsd) (mainly to be able to fuse all examples of em-specific data converters including new ebsd examples into one em parser for pynxtools), ii) refactor NXem which is now clearly a specific appdef specializing the NXem_base deep base class, specialization work needs to define which fields and groups from all the possible ones now composable via base classes (inheritance) are required in an appdef NXem for NOMAD OASIS
… group inside the NXem application definition, next step: i) refactor NXem_sim, ii) finalize refactoring of NXem appdef (for Nomad oasis)
@mkuehbach mkuehbach mentioned this pull request Aug 14, 2023
markus.kuehbach added 6 commits August 14, 2023 12:56
…and dimensions could be added to nyamlforward
…ng to explore further usage of refactored EM base classes. Info keyword has to be a child of doc or the respective text be removed from the standard and put into proposal-specific documentation, how to store what and where so that the schema docstring remain succinct and slim but all these conceptual ideas get not forgotten, typically the would be part of a tech report, i.e. in my opinion all what is under info: sections of a docstring should move to some documentation to tell the story to humans, next test these NXDLs with the NeXus documentation system
…ow on the refactored EM, there are still some spurious info fields now, which should be removed when a decision has been made wrt to how to deal with info: keyword fields in general, next steps: i) make decision on info, ii) test refactored EM proposal with pynxtools em-parser v2, iii) implement backwards
@mkuehbach
Copy link
Collaborator Author

mkuehbach commented Aug 14, 2023

Nothing more currently from my side, some spurious black style issues (fixed now), but rather I would like to refactor the em parsers now with this, decisions on keyword info, readthedocs need to be made.

…r CG and MS based on suggestions from user meetings, discussions with Sandor, represents work with the MPES sprint #83
@mkuehbach mkuehbach marked this pull request as ready for review November 10, 2023 13:38
mkuehbach added 12 commits November 10, 2023 22:33
…nch based on 1016aa0, NXms_recon has still an issue and is therefore deactivated currently, method-specific landing pages need to be updated
…61a8d0cb62eccf53, removed, modified by taking the one from fairmat, and synced all files which were binarily different between this feature branch and the fairmat branch specifically commit a15798b of the fairmat branch
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.

suggestion for using NX_DATE_TIME (instead of NX_CHAR) has been left. Other than that LGTM

contributed_definitions/NXcg_point_set.nxdl.xml Outdated Show resolved Hide resolved
@sanbrock sanbrock merged commit 1d0d045 into fairmat Nov 30, 2023
6 checks passed
@lukaspie lukaspie deleted the base_class_templates branch February 13, 2024 12:40
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

2 participants