-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
Codecov ReportAttention: Patch coverage is
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. |
f66bee7
to
8058188
Compare
There was a problem hiding this 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.
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. |
c4fafdd
to
73e6eee
Compare
73e6eee
to
2f6e5b4
Compare
Co-authored-by: Jonas Lähnemann <jonas@pdi-berlin.de>
@ericpre yes, I am currently working on it |
I missread your comment. I want to try it by myself first and I will ask you when I cannot make it work |
2f6e5b4
to
2be342c
Compare
@ericpre While rebasing it came to my attention that occasionally constructs such as
were replaced by
Why is this done only occasionally? Should this be replaced in all places instead? |
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. |
ok, thanks for the clarification |
There was a problem hiding this 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!
record_by
metadata
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 thesignal_dimension
, as thenavigate
attribute wasn't set for the axes.Now
navigate
is set for all readers so thatrecord_by
can be removed.Progress of the PR
upcoming_changes
folder