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

Switch to Multiformat Reader - Witec and ROD #16

Merged
merged 14 commits into from
Dec 10, 2024

Conversation

RonHildebrandt
Copy link
Collaborator

@RonHildebrandt RonHildebrandt commented Dec 9, 2024

Somehow, something unknown broke the the CI/CD broke with the first PR version. Here a new PR:

First version to add the Raman Open Database File reader.

Tasks

  • assign all fields via config file
  • generalize reader to work with all present example (witec .yaml+.txt AND .rod file)
  • figure out fields, which can not be directly assigned - take a look into ROD documentation | Some information can be found in the README file for the .rod example.
  • Make Issue to remove the assignment of "single-channel" for the "detector_channel_type" in NXoptical_spectroscopy avoid pytest warnings. This requirement will be relaxed, as this will be relaxed later in the nexus definitions

@RonHildebrandt RonHildebrandt self-assigned this Dec 10, 2024
@RonHildebrandt RonHildebrandt marked this pull request as ready for review December 10, 2024 09:06
@RonHildebrandt
Copy link
Collaborator Author

I want to update later the .rod file parser, to include all available meta data - with a seperate PR.

@RonHildebrandt
Copy link
Collaborator Author

I have now included the config files as well wihtin the tests. Would be better if they are only at one location.

Maybe I can remove then the config folder in source? As this config file is spcified for the respective example, it may be better to place it there?

@RonHildebrandt RonHildebrandt mentioned this pull request Dec 10, 2024
5 tasks
@lukaspie
Copy link
Collaborator

I have now included the config files as well wihtin the tests. Would be better if they are only at one location.

Maybe I can remove then the config folder in source? As this config file is spcified for the respective example, it may be better to place it there?

The point of having it in src/ is that you don't need to always use it as an input file to the reader. If you remove it from there, you e.g. always have to drop it to NOMAD. I thought the rod data has always the same structure? Then it makes sense to have a config as default. You can always drop another config file that takes priority over the default.

By the way, when I remove the config file in tests/data/rod, the tests still work. But not so for removing it from tests/data/witec

@RonHildebrandt
Copy link
Collaborator Author

RonHildebrandt commented Dec 10, 2024

@lukaspie
"The point of having it in src/ is that you don't need to always use it as an input file to the reader. If you remove it from there, you e.g. always have to drop it to NOMAD. I thought the rod data has always the same structure? Then it makes sense to have a config as default. You can always drop another config file that takes priority over the default."
Ah alrigtht. Then I want to keep it that way. Sounds legit.

"By the way, when I remove the config file in tests/data/rod, the tests still work. But not so for removing it from tests/data/witec"

Hm okay? Ill try to take a look into it. Weird.
UPDATE. Okay found it. Adjusted the witec code and removed all other not needed config files.

Copy link
Collaborator

@lukaspie lukaspie left a comment

Choose a reason for hiding this comment

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

Left some comments here and there, but overall looks okay for me now. Please also update the name and description of the PR for future reference.

src/pynxtools_raman/reader.py Outdated Show resolved Hide resolved
src/pynxtools_raman/reader.py Outdated Show resolved Hide resolved
src/pynxtools_raman/reader.py Show resolved Hide resolved
src/pynxtools_raman/reader.py Outdated Show resolved Hide resolved
src/pynxtools_raman/reader.py Outdated Show resolved Hide resolved
src/pynxtools_raman/rod/rod_reader.py Outdated Show resolved Hide resolved
src/pynxtools_raman/rod/rod_reader.py Outdated Show resolved Hide resolved
src/pynxtools_raman/rod/rod_reader.py Outdated Show resolved Hide resolved
@RonHildebrandt RonHildebrandt changed the title initial_commit Switch to Multiformat Reader - Witec and ROD Dec 10, 2024
@RonHildebrandt RonHildebrandt merged commit f9e5e7a into main Dec 10, 2024
6 checks passed
@lukaspie lukaspie deleted the cicd_test_make_raman_test branch December 10, 2024 12:54
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.

2 participants