FAIRmat-NFDI / nexus_definitions

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

Fixes nyaml.mk #189

Closed domna closed 4 months ago

domna commented 4 months ago

@mkuehbach and @RonHildebrandt: Could you please check if this works for you now?

domna commented 4 months ago

I made a clean install of this branch and ran make nxdl first then make nyaml, both commands worked out of the box. In effect, many NXDL files where showing up with changes but that is expected because of the inconsistencies we still have on the fairmat branch.

I rebase it, so it should not produce any change nxdl files. For me a running make nyaml and then make nxdl doesn't create any changed nxdl files now. @mkuehbach Could you check with the latest commit?

It does create a bunch of changed yaml files, though. But this is expected.

Am I right that for a clean install one should first run "make nyaml" and once one has edited them run "make nxdl", @domna, @sanbrock ?

I think the important route is that make nxdl does not create any changes to the nxdl's.

sanbrock commented 4 months ago

Yes, make nyaml is the first command, because normally there should not be nyaml files committed to the repo. thean after changing the nyaml files, one shall do make nxdl to commit the changes in nxdl.

In the past we wanted to store definitions in our nyaml dialects, because the tool did not properly translated the content to nxdl, so we did not want to loose information. At that time, it was imporntant to highlight if the committed nxdl files did not match the committed nyaml files.

lukaspie commented 4 months ago

Yes, make nyaml is the first command, because normally there should not be nyaml files committed to the repo. thean after changing the nyaml files, one shall do make nxdl to commit the changes in nxdl.

In the past we wanted to store definitions in our nyaml dialects, because the tool did not properly translated the content to nxdl, so we did not want to loose information. At that time, it was imporntant to highlight if the committed nxdl files did not match the committed nyaml files.

We have a CI workflow that checks that the nyaml/nxdl files are consistent. Not committing the nyaml files makes this check useless. We should discuss again if we want to have the nyaml files in the repo or not. I think it's fine to have them as long as the nyaml version which created them is fixed in the requirements.

mkuehbach commented 4 months ago

Locally I have 8e9f76 "Adds tabs to nyaml.mk" i.e. the latest, after running make nyaml and make nxdl consecutively many yaml files are different with one prominent git diff being that instead of using dim: (3,) we use again the old dimensions syntax (in the yaml file)

I see there are at least three related but different points: i) the fix-nyamlmk rightly so fixes the conversion, ii) we have still inconsistencies of NXDL and YAMLs on fairmat, iii) the round-trip conversion from yaml to nxdl returns at least to my understanding confusion behaviour: If I state in a YAML file that using dim: suffices it is alright that this is expanded in the NXDL.XML file but if in this expanded version then all childs of dimensions use only indices and value the default should be that in yaml dim: is exported. So I think point iii) is that the default conversion behaviour is at least different than expected making de facto the auto conversion of nxdl to yaml unnecessarily verbose for dimensions

mkuehbach commented 4 months ago

My proposal is:

  1. Remove all changes on files other than the mk file in this PR
  2. Then merge this PR
  3. Merge other relevant PR (discuss at TF today)
  4. Create a new niac_consistency_ensuring or whatever named branch where we all work to solve a) point ii) and point iii) mentioned above and go again through the official NIAC versions of appdefs and base classes and check that we have them included in our XML files then
  5. Merge that niac_consistency_ensuring branch into fairmat Then I feel finally save enough to start working on the plugin implementation for em and apm.

Thoughts @domna , @lukaspie , @sanbrock ?