FAIRmat-NFDI / nomad-measurements

A NOMAD plugin containing base sections for measurements.
https://FAIRmat-NFDI.github.io/nomad-measurements/
Apache License 2.0
14 stars 1 forks source link

app def missing. #108

Closed RubelMozumder closed 3 months ago

RubelMozumder commented 4 months ago

@budschi, @ka-sarthak and @sherjeelshabih , Now the nexus parts are available in EntryArchive.json. But it needs the MR in gtilab merged to the develop branch.

The next task is to map the full data path from archive to nexus and vice-versa. Once @ka-sarthak comes from holidays we can map all the data and merge it.

ka-sarthak commented 4 months ago

@RubelMozumder some comments from our meeting today:

ka-sarthak commented 4 months ago

@RubelMozumder After discussing with @hampusnasstrom, we think it's okay to have an entry archive for the corresponding .nxs file. It indexes the .nxs file for the nomad search.

The important thing is that data should not be duplicated. But I think this is already avoided because both the nexus sub-section and the entry archive based on .nxs file do not contain the data, but references to the quantities in .nxs file. Right?

As such, the only thing remaining to resolve is that the entry type of ELNXRayDiffraction should not be modified.

ka-sarthak commented 4 months ago

@RubelMozumder FYI I rebased the write-nexus-section branch, but now your branch has some merge conflicts

RubelMozumder commented 4 months ago

@RubelMozumder FYI I rebased the write-nexus-section branch, but now your branch has some merge conflicts

I can fix it.

RubelMozumder commented 4 months ago

@RubelMozumder some comments from our meeting today:

  • Entry type of ELNXRayDiffraction should not be modified
  • ELNXRayDiffraction should have a nexus sub-section
  • additional .nxs file can be created
  • additional entry based on the .nxs file should be avoided

All requested features are implemented, here. A short overview:

  1. Nexus hdf5 file can be created as an entry or a simple file using boolean arg: nxs_as_entry.
  2. The schema type or entry_type ELNXRayDiffraction is not overwritten anymore.

I separated the related code into a file nx.py in the interest of code readability and maintainability.

RubelMozumder commented 3 months ago

@ka-sarthak, I think it should be ready to merge, now.

ka-sarthak commented 3 months ago

@RubelMozumder I tried running the branch in my local nomad installation. For some reason, the entry associated with the .nxs file is not being generated. Didn't you implement this? Or do I need to add nexus parsers for it to work?

ka-sarthak commented 3 months ago

I have added some comments. Once you resolve it, please do the following things:

One issue that remains is: m_def of an entry is being overwritten by populate_nexus_subsection. Your current fix to re-overwrite it again works for now, but we need to be sure that other metadata is not being altered. As suggested, we can add a TODO there to close this PR and return to it later.

But otherwise, looks great! Thanks for the amazing work!

RubelMozumder commented 3 months ago

I have added some comments. Once you resolve it, please do the following things:

  • run the ruff autoformatter on nx.py file (because this is newly added code, better to be formatted)
  • remove all the formatting from schema.py (because this is old code, and formatting will be handled elsewhere)

One issue that remains is: m_def of an entry is being overwritten by populate_nexus_subsection. Your current fix to re-overwrite it again works for now, but we need to be sure that other metadata is not being altered. As suggested, we can add a TODO there to close this PR and return to it later.

But otherwise, looks great! Thanks for the amazing work!

@ka-sarthak, I removed the entry_type overwriting part. I also tested it and from my side it is ok. Would you please test it to see if it is okay with you (in case I overlooked anything)?

please, keep in mind to test the changes in pynxtools you have to try from the master branch from https://github.com/FAIRmat-NFDI/pynxtools. A new release will be available by tomorrow.

ka-sarthak commented 3 months ago

@RubelMozumder As suggested by @aalbino2 , I just rebased the write-nexus-section on 101-update-package-structure to include the migration to the new plugin mechanism. I also rebased the current PR branch on write-nexus-section to include those changes here.

now if you want to test the plugin in your local installation, make sure to use the new plugin mechanism to load the plugin.

ka-sarthak commented 3 months ago

@RubelMozumder entry_type of ELNXRayDiffraction is getting modified when the nexus section is populated. it seems that the changes you made in pynxtools does not fix this issue. can you take another look?

ka-sarthak commented 3 months ago

or maybe it's that pynxtools version need to be updated in pyproject

aalbino2 commented 3 months ago

thanks @ka-sarthak

Hey @RubelMozumder we can look at the rebase together if you need, let me know

ka-sarthak commented 3 months ago

@RubelMozumder The code works nicely for 1D line scans. But it throws an error for RSM scan coming from nx.py file:

Traceback (most recent call last):
  File "/home/sarthak-kapoor/repositories/fairmat/nomad-FAIR/nomad/normalizing/metainfo.py", line 38, in normalize_section
    normalize(archive, logger)
  File "/home/sarthak-kapoor/repositories/fairmat/nomad-measurements/src/nomad_measurements/xrd/schema.py", line 983, in normalize
    write_nx_section_and_create_file(archive, logger, scan_type=scan_type)
  File "/home/sarthak-kapoor/repositories/fairmat/nomad-measurements/src/nomad_measurements/xrd/nx.py", line 174, in write_nx_section_and_create_file
    connect_concepts(template, archive, scan_type=scan_type)
  File "/home/sarthak-kapoor/repositories/fairmat/nomad-measurements/src/nomad_measurements/xrd/nx.py", line 121, in connect_concepts
    if archive_concept.endswith('units')
       ^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'dict' object has no attribute 'endswith'

The error shows up in the GUI and not when using the CLI.