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

Remove deprecated record_by metadata #102

Merged
merged 7 commits into from
Jun 2, 2023

Conversation

pietsjoh
Copy link
Contributor

@pietsjoh pietsjoh commented Apr 19, 2023

Description of the change

This PR solves #79, removing the deprecated record_by metadata attribute for the affected file readers.

record_by was used in these cases to infer the signal_dimension, as the navigate attribute wasn't set for the axes.
Now navigate is set for all readers so that record_by can be removed.

Progress of the PR

  • Remove record_by metadata attribute
    • blockfile
    • bruker
    • digitalmicrograph
    • edax
    • empad
    • image
    • jeol
    • mrc
    • msa
    • netcdf
    • ripple
    • tia
    • tiff
  • add an changelog entry in the upcoming_changes folder
  • add tests
    • bruker
    • digitalmicrograph
    • ripple
    • tiff
  • ready for review.

@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Attention: Patch coverage is 82.60870% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.01%. Comparing base (ff86cce) to head (2be342c).
Report is 548 commits behind head on main.

Files with missing lines Patch % Lines
rsciio/netcdf/_api.py 0.00% 2 Missing ⚠️
rsciio/mrc/_api.py 0.00% 1 Missing ⚠️
rsciio/ripple/_api.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
+ Coverage   85.00%   85.01%   +0.01%     
==========================================
  Files          73       73              
  Lines        9054     9062       +8     
  Branches     2049     2049              
==========================================
+ Hits         7696     7704       +8     
- Misses        892      893       +1     
+ Partials      466      465       -1     

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

@jlaehne jlaehne linked an issue Apr 19, 2023 that may be closed by this pull request
@jlaehne jlaehne added this to the v0.1.0 initial release milestone Apr 19, 2023
@jlaehne jlaehne requested a review from ericpre April 25, 2023 05:53
@jlaehne
Copy link
Contributor

jlaehne commented Apr 25, 2023

Could you merge in main @pietsjoh to see whether #106 fixed the currently failing tests?

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.
Can please you add tests to check that the signal and navigation are correct for the following reader:

  • bruker
  • digitalmicrograph
  • ripple
  • tiff

There are no tests for mrc... but that should be sorted in a separate PR.

@pietsjoh
Copy link
Contributor Author

Can please you add tests to check that the signal and navigation are correct for the following reader

Yes. However, this depends on the existing testfiles. For example tia has no tests for images/maps according to the reader. I will have a look at the tests/testfiles for these readers and I am going to try to cover the different axes sizes.

@ericpre
Copy link
Member

ericpre commented Jun 2, 2023

@pietsjoh, do you want me to help with the rebase? The conflicts should come from #123 and the change to the path of the test files, etc..

@pietsjoh
Copy link
Contributor Author

pietsjoh commented Jun 2, 2023

@ericpre yes, I am currently working on it

@pietsjoh
Copy link
Contributor Author

pietsjoh commented Jun 2, 2023

I missread your comment. I want to try it by myself first and I will ask you when I cannot make it work

@pietsjoh
Copy link
Contributor Author

pietsjoh commented Jun 2, 2023

@ericpre While rebasing it came to my attention that occasionally constructs such as

def test_example():
    with tempfile.TemporaryDirectory() as tmpdir:
        fname = os.path.join(tmpdir, "tiff_files", "filename.tif")
        s.save(fname, overwrite=True, export_scale=True)``

were replaced by

def test_example(tmp_path):
    s = hs.load(tmp_path / "test_rgba16.tif")

Why is this done only occasionally? Should this be replaced in all places instead?

@ericpre
Copy link
Member

ericpre commented Jun 2, 2023

Indeed, this hasn't done everywhere because it is not possible to use the same pattern, see for example: #123 (comment) and #123 (comment).

There were already a lot of changes in that PR, so I have left this part as it was.

@pietsjoh
Copy link
Contributor Author

pietsjoh commented Jun 2, 2023

ok, thanks for the clarification

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.

Thank you, that's great!

@ericpre ericpre merged commit 1491e6c into hyperspy:main Jun 2, 2023
@ericpre ericpre changed the title Deprecate record_by metadata attribute Remove deprecated record_by metadata Jul 14, 2023
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.

Deprecate record_by metadata setting
3 participants