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

Add sub-reader for PHI Versaprobe data #5

Merged
merged 36 commits into from
Mar 28, 2024

Conversation

lukaspie
Copy link
Collaborator

@lukaspie lukaspie commented Feb 20, 2024

Phi Parser:

  • Parser for PHI Versaprobe .spe (regular spectra) and .pro (sputter depth profiling)
  • New config file to account for these data sets
  • Correct parsing of binary encoded data
  • Mapping of optional instrument devices (sputter source, neutral ion gun, C60 gun, defect positioner) to the config file

Other changes:

  • The main reader supports energy reading as well as more options for writing raw detector data (i.e., write scan data if there is no data for the individual channels

Error fixes:

  • Links that are broken (see [Bug]: Handling of links #2) are temporarily removed from config files until that bug is fixed.
  • The mapping of keys and the renaming of ENTRY[entry] to the actual entries (e.g. `ENTRY[MeasurementGroup_Survey]) has been streamlined and harmonized across all subreaders

Still missing:

  • Example data for this reader (pending approval by data provider)
  • Test data for this reader (pending approval by data provider)

This PR depends on FAIRmat-NFDI/pynxtools#220 and should be merged afterwards/together.
EDIT: FAIRmat-NFDI/pynxtools#220 merged into pynxools-xps@main.

Resolves #1.

@lukaspie lukaspie linked an issue Feb 20, 2024 that may be closed by this pull request
@lukaspie lukaspie changed the title Add XPS sub-reader for PHI Versaprobe data Add sub-reader for PHI Versaprobe data Feb 22, 2024
@lukaspie lukaspie added enhancement New feature or request sub-reader labels Mar 4, 2024
Copy link
Contributor

github-actions bot commented Mar 18, 2024

Pull Request Test Coverage Report for Build 8465335526

Details

  • 704 of 731 (96.31%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+10.0%) to 58.792%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pynxtools_xps/sle/sle_specs.py 0 1 0.0%
pynxtools_xps/txt/txt_vamas_export.py 0 1 0.0%
pynxtools_xps/txt/txt_scienta.py 0 2 0.0%
pynxtools_xps/vms/vamas.py 3 13 23.08%
pynxtools_xps/phi/spe_pro_phi.py 328 341 96.19%
Totals Coverage Status
Change from base Build 8427529939: 10.0%
Covered Lines: 1956
Relevant Lines: 3327

💛 - Coveralls

@lukaspie lukaspie force-pushed the 1-add-xps-sub-reader-for-phi-versaprobe-data branch from 6661351 to b333a6d Compare March 18, 2024 15:41
@lukaspie lukaspie requested review from RubelMozumder and domna March 26, 2024 08:42
@lukaspie lukaspie mentioned this pull request Mar 26, 2024
5 tasks
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.

There are some small things and my general confusion about the dict (whether this can be a config file). But probably there is a good reason this is separate from the config file which I just don't understand.

I tested it with the files from Sebastian and it all works well.

An additional consideration we already discussed in discord is adding an additional axis for the depth profile so you can view it in one NXdata. Depending on the effort this can either be handled here or in a separate PR.

Thanks for the implementation :)

examples/phi/README.md Outdated Show resolved Hide resolved
pynxtools_xps/phi/spe_pro_phi.py Show resolved Hide resolved
pynxtools_xps/phi/spe_pro_phi.py Outdated Show resolved Hide resolved
pynxtools_xps/phi/spe_pro_phi.py Show resolved Hide resolved
pynxtools_xps/phi/spe_pro_phi.py Outdated Show resolved Hide resolved
pynxtools_xps/reader_utils.py Outdated Show resolved Hide resolved
pynxtools_xps/reader_utils.py Outdated Show resolved Hide resolved
@lukaspie
Copy link
Collaborator Author

An additional consideration we already discussed in discord is adding an additional axis for the depth profile so you can view it in one NXdata. Depending on the effort this can either be handled here or in a separate PR.

I realized that this is a bit of an involved effort because the XPS reader so far only works for one-dimensional datasets with an energy axis. In principle, I think I should redesign it such that not every scan gets an axis in NXdata, but rather the scan numbers, measurement cycle numbers, sputter cycle numbers, are axes to one NXdata. However, this requires quite a bit of effort to disentangle in the main reader, so I suggest that I will do it in a separate PR.

Thus, I would just simply wait for the approval that the example/test data can be included here and then it would merge this PR.

@domna
Copy link
Collaborator

domna commented Mar 28, 2024

Thus, I would just simply wait for the approval that the example/test data can be included here and then it would merge this PR.

Sounds reasonable. I'll approve this already. Feel free to add the test data and merge when ready

@lukaspie lukaspie merged commit b6a924f into main Mar 28, 2024
6 checks passed
@lukaspie lukaspie deleted the 1-add-xps-sub-reader-for-phi-versaprobe-data branch March 28, 2024 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request sub-reader
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sub-reader for PHI Versaprobe data
2 participants