8000 Merging of multiple files by sherjeelshabih · Pull Request #176 · FAIRmat-NFDI/pynxtools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
8000

Merging of multiple files #176

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Nov 21, 2023
Merged

Merging of multiple files #176

merged 13 commits into from
Nov 21, 2023

Conversation

sherjeelshabih
Copy link
Collaborator
@sherjeelshabih sherjeelshabih commented Nov 1, 2023 •
edited
Loading

I have broken down an NXiv_temp NeXus file.

@sherjeelshabih sherjeelshabih linked an issue Nov 1, 2023 that may be closed by this pull request
Merging of multiple files #165
Closed
@sherjeelshabih sherjeelshabih self-assigned this Nov 1, 2023
@coveralls
Copy link
coveralls commented Nov 1, 2023 •
edited by github-actions bot
Loading

Pull Request Test Coverage Report for Build 6944969384

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 25 of 55 (45.45%) changed or added relevant lines in 4 files are covered.
  • 59 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+0.2%) to 51.492%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pynxtools/dataconverter/convert.py 4 6 66.67%
pynxtools/dataconverter/hdfdict.py 0 5 0.0%
pynxtools/dataconverter/readers/json_map/reader.py 20 43 46.51%
Files with Coverage Reduction New Missed Lines %
pynxtools/dataconverter/readers/shared/map_concepts/mapping_functors.py 1 11.86%
pynxtools/nyaml2nxdl/nyaml2nxdl_forward_tools.py 1 79.42%
tests/dataconverter/test_writer.py 1 96.77%
pynxtools/nexus/nxdl_utils.py 2 74.95%
tests/dataconverter/test_helpers.py 3 97.84%
pynxtools/dataconverter/template.py 5 91.38%
pynxtools/dataconverter/convert.py 6 83.18%
pynxtools/init.py 7 76.67%
pynxtools/dataconverter/helpers.py 14 96.01%
pynxtools/dataconverter/readers/ellips/reader.py 19 84.95%
Totals Coverage Status
Change from base Build 6668400701: 0.2%
Covered Lines: 6178
Relevant Lines: 11998
💛 - Coveralls

@domna domna mentioned this pull request Nov 2, 2023
Merged
@sherjeelshabih
Copy link
Collaborator Author
sherjeelshabih commented Nov 7, 2023 •
edited
Loading
  • Add merge and map as CLI options piping those calls through the JSON Map Reader
  • Add docs for the new options

domna
domna reviewed Nov 8, 2023
View reviewed changes
Copy link
Collaborator
@domna domna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works nicely. Except from a view things this looks nice already. I have two requests though (which we might also move to separate issues if they are not already) which might be quick to do for you:

  • Do we also want to add writing @partial files in this PR? I think this would also be a needed feature, but I think we don't have a separate issue for this yet.
  • Could you write a small tutorial/how to for this feature? Just an mkdocs md file and then we can integrate it where we need it (nomad, future pynxtools mkdocs?). It doesn't need to be supper eloquent but I think it's good to have a basis as this will be very relevant for the technology partners. https://github.com/FAIRmat-NFDI/nexus_definitions/issues/110

domna
domna approved these changes Nov 21, 2023
View reviewed changes
Copy link
Collaborator
@domna domna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the additional docs. The adding of the @partial should be handled in #175

@lukaspie
Copy link
Collaborator

Sorry for the delay. I just tested all the new features and didn't find any bugs. I agree that group copying might become an important feature. As an example if I have an instrument in Nomad which complies to a certain nxdl version and I also want to use this in another experiment, I could just copy all the static instrument groups (e.g. NXtransformations) over, without needing to map every single detail. But, this is out of the scope of this PR, so let's add it later.

@sherjeelshabih sherjeelshabih merged commit 4cc45d3 into master Nov 21, 2023
@sherjeelshabih sherjeelshabih deleted the 165-merging-of-multiple-files branch January 15, 2024 15:50
@sherjeelshabih
Copy link
Collaborator Author
sherjeelshabih commented Feb 14, 2024 •
edited
Loading

Fixes FAIRmat-NFDI/nexus_definitions#164 FAIRmat-NFDI/nexus_definitions#165

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domna domna domna approved these changes

@lukaspie lukaspie lukaspie approved these changes

Assignees

@sherjeelshabih sherjeelshabih

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merging of multiple files
4 participants
@sherjeelshabih @coveralls @lukaspie @domna
0