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

Fix error when reading pts file with frame_list, frame_start_index (was PR #67) #68

Merged
merged 5 commits into from
Dec 1, 2022

Conversation

nem1234
Copy link
Contributor

@nem1234 nem1234 commented Nov 22, 2022

Description of the change

  • Bugfix : error when reading JEOL .pts file with un-ordered frame list
  • Bugfix : error when length of frame_start_index is smaller than sweep count
  • Revise warning message
  • Add test for frame list, frame_shift and frame_start_index
  • Add test for broken asw file
  • Add test for SEM data

Progress of the PR

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

Minimal example of the bug fix or the new feature

s = hs.load("file.pts", frame_list=[1,3])
frame_start_index = s.original_metadata.jeol_pts_frame_start_index
s = hs.load("file.pts", frame_list=[5,2], frame_start_index=frame_start_index)

Add tests for frame_start_index and frame_shift
@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Base: 83.52% // Head: 84.11% // Increases project coverage by +0.58% 🎉

Coverage data is based on head (d222c57) compared to base (055822e).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
+ Coverage   83.52%   84.11%   +0.58%     
==========================================
  Files          49       56       +7     
  Lines        8390     8411      +21     
  Branches     1893     1890       -3     
==========================================
+ Hits         7008     7075      +67     
+ Misses        909      877      -32     
+ Partials      473      459      -14     
Impacted Files Coverage Δ
rsciio/jeol/api.py 99.13% <100.00%> (+12.17%) ⬆️
rsciio/netcdf/api.py
rsciio/phenom/api.py
rsciio/protochips/api.py
rsciio/ripple/api.py
rsciio/nexus/api.py
rsciio/prz/api.py
rsciio/phenom/__init__.py 100.00% <0.00%> (ø)
rsciio/ripple/__init__.py 100.00% <0.00%> (ø)
rsciio/netcdf/__init__.py 100.00% <0.00%> (ø)
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

rsciio/jeol/api.py Fixed Show fixed Hide fixed
@jlaehne
Copy link
Contributor

jlaehne commented Nov 22, 2022

Continuation of #67

@nem1234 nem1234 force-pushed the bugfix_jeol_pts_framelist branch 4 times, most recently from 512628c to a0d5b00 Compare November 24, 2022 10:34
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.

It looks good.

Do you want to add a release note mentioning the following?

  • Bugfix : error when reading JEOL .pts file with un-ordered frame list
  • Bugfix : error when length of frame_start_index is smaller than sweep count

@nem1234
Copy link
Contributor Author

nem1234 commented Nov 30, 2022

Thank you.
This error seems very rare cases, so I don't think adding release note is needed.

By the way, if it is needed, should I write issue before adding upcoming_changes?
Is unnumbered log allowed?

@ericpre
Copy link
Member

ericpre commented Nov 30, 2022

Based on my experience, release notes are useful even for the most minor changes/bug fix, as it is only useful for users but also for ourselves in the future and developers of other libraries.

To add a release note, you just need to add a file in the upcoming_changes folder with the correct name (PR or issue number and type of changes) and then towncrier will process the entry automatically (add the release note in the right place and add create the hyperlink).

@nem1234
Copy link
Contributor Author

nem1234 commented Nov 30, 2022

Ok, I put 68.bugfix.rst into upcoming_change directory

@ericpre ericpre merged commit 7a34b06 into hyperspy:main Dec 1, 2022
@jlaehne jlaehne added this to the v0.1.0 initial release milestone Dec 1, 2022
@nem1234 nem1234 deleted the bugfix_jeol_pts_framelist branch December 2, 2022 02:34
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.

3 participants