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

File_Writer for DigitalSurf files #280

Merged
merged 24 commits into from
Jul 4, 2024
Merged

Conversation

Attolight-NTappy
Copy link
Contributor

@Attolight-NTappy Attolight-NTappy commented Jun 21, 2024

Description of the change

This PR implements save support for digitalsurf file formats:

  • Implement file_writer for the digitalsurf formats (.sur and .pro)
  • Correct a minor bug causing some datasets being offset by a constant upon loading
  • Improve documentation

Progress of the PR

  • Change implemented (can be split into several points),
  • update docstring (if appropriate),
  • update user guide (if appropriate),
  • add a changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
  • Check formatting of the changelog entry (and eventual user guide changes) in the docs/readthedocs.org:rosettasciio build of this PR (link in github checks)
  • add tests,
  • ready for review.

Minimal example of the bug fix or the new feature

import numpy as np
from rsciio.digitalsurf import file_writer,file_reader
md = { 'General': {},
       'Signal': {}}

ax  = {'name': 'X',
 'navigate': False,
 }

sd = {"data": np.arange(24)+111,
       "axes": [ax],
       "metadata": md,
       "original_metadata": {}}

file_writer("test.pro",sd)
file_reader('test.pro')[0]['data']
#array([111., 112., 113., 114., 115., 116., 117., 118., ... 133., 134.])

rsciio/digitalsurf/_api.py Fixed Show resolved Hide resolved
rsciio/digitalsurf/_api.py Fixed Show resolved Hide resolved
rsciio/digitalsurf/_api.py Fixed Show resolved Hide resolved
Copy link

codecov bot commented Jun 21, 2024

Codecov Report

Attention: Patch coverage is 97.51773% with 14 lines in your changes missing coverage. Please review.

Project coverage is 87.66%. Comparing base (e921e77) to head (1f5e4c5).
Report is 85 commits behind head on main.

Files with missing lines Patch % Lines
rsciio/digitalsurf/_api.py 97.50% 0 Missing and 14 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #280      +/-   ##
==========================================
+ Coverage   86.33%   87.66%   +1.33%     
==========================================
  Files          83       83              
  Lines       10672    11150     +478     
  Branches     2331     2414      +83     
==========================================
+ Hits         9214     9775     +561     
+ Misses        939      860      -79     
+ Partials      519      515       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rsciio/digitalsurf/_api.py Fixed Show fixed Hide fixed
rsciio/digitalsurf/_api.py Fixed Show fixed Hide fixed
rsciio/digitalsurf/_api.py Fixed Show fixed Hide fixed
rsciio/tests/test_digitalsurf.py Dismissed Show resolved Hide resolved
rsciio/tests/test_digitalsurf.py Fixed Show resolved Hide resolved
rsciio/digitalsurf/_api.py Fixed Show resolved Hide resolved
rsciio/tests/test_digitalsurf.py Fixed Show resolved Hide resolved
@Attolight-NTappy
Copy link
Contributor Author

This is now ready for review :)

@ericpre
Copy link
Member

ericpre commented Jun 28, 2024

pre-commit.ci autofix

Copy link
Member

@ericpre ericpre left a comment

Choose a reason for hiding this comment

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

This looks good, I didn't review in depth but this looks all sensible.
I made a few comments and maybe it would be good to increase the coverage? There are a couple of if block which are not coverage.

doc/user_guide/supported_formats/digitalsurf.rst Outdated Show resolved Hide resolved
rsciio/digitalsurf/Untitled-1.ipynb Outdated Show resolved Hide resolved
rsciio/digitalsurf/_api.py Outdated Show resolved Hide resolved
rsciio/digitalsurf/_api.py Show resolved Hide resolved
rsciio/tests/test_digitalsurf.py Show resolved Hide resolved
rsciio/tests/test_digitalsurf.py Show resolved Hide resolved
rsciio/tests/data/digitalsurf/test_isurface.sur Outdated Show resolved Hide resolved
fixed type

Co-authored-by: Eric Prestat <eric.prestat@gmail.com>
@Attolight-NTappy
Copy link
Contributor Author

Thanks for review!

I made a few comments and maybe it would be good to increase the coverage? There are a couple of if block which are not coverage.

Alright, wait for it I'll push some new tests later today

Copy link
Member

@ericpre ericpre left a comment

Choose a reason for hiding this comment

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

A test checking the list of functions in the public API needs to be updated:

if plugin["name"] == "MSA":
assert dir(plugin_module) == [
"file_reader",
"file_writer",
"parse_msa_string",
]
elif plugin["name"] == "QuantumDetector":
assert dir(plugin_module) == [
"file_reader",
"load_mib_data",
"parse_exposures",
"parse_timestamps",
]
elif plugin["writes"] is False:
assert dir(plugin_module) == ["file_reader"]
else:
assert dir(plugin_module) == ["file_reader", "file_writer"]

rsciio/digitalsurf/_api.py Outdated Show resolved Hide resolved
@ericpre
Copy link
Member

ericpre commented Jul 4, 2024

parse_metadata needs to be part of the public API, this is the test that needs to be updated to take into account this change.

@Attolight-NTappy
Copy link
Contributor Author

Ok. I thought you would not consider changing this. Hadn't realized some other modules where already deviating from the file_reader,file_writer norm
my bad

@Attolight-NTappy
Copy link
Contributor Author

Ok finally I think we're there.

@ericpre ericpre merged commit 2c33d09 into hyperspy:main Jul 4, 2024
31 of 32 checks passed
@ericpre
Copy link
Member

ericpre commented Jul 4, 2024

Thank you @Attolight-NTappy, I push some tweaks to please ruff and improve the release note.

@Attolight-NTappy
Copy link
Contributor Author

🤩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants