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

Check existence of signal.metadata.Markers before deleting it (was PR #3070) #3091

Merged
merged 5 commits into from Jan 30, 2023

Conversation

nem1234
Copy link
Contributor

@nem1234 nem1234 commented Jan 26, 2023

Description of the change

  • Do not raise AttributeError when loading markers

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

This error does not shown in RELEASE_next_major branch, but potentially raised when broken file reader is loaded.

@codecov
Copy link

codecov bot commented Jan 26, 2023

Codecov Report

Base: 80.60% // Head: 80.64% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (aad704b) compared to base (b7b6f6d).
Patch coverage: 75.00% of modified lines in pull request are covered.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           RELEASE_next_major    #3091      +/-   ##
======================================================
+ Coverage               80.60%   80.64%   +0.03%     
======================================================
  Files                     176      176              
  Lines                   24476    24474       -2     
  Branches                 5383     5383              
======================================================
+ Hits                    19729    19737       +8     
+ Misses                   3421     3412       -9     
+ Partials                 1326     1325       -1     
Impacted Files Coverage Δ
hyperspy/io.py 66.56% <75.00%> (+1.04%) ⬆️
hyperspy/signal.py 77.67% <0.00%> (+0.26%) ⬆️

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.

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.

I made some comment and it would be good to add a test along the line of:

import hyperspy.api as hs
import numpy as np

s = hs.signals.Signal1D(np.arange(10))
m = hs.plot.markers.Point(x=5, y=5)
s.add_marker(m, permanent=True)

fname = "test.hspy"

s.save(fname)
s2 = hs.load(fname)
assert s.metadata.Markers == ...
s2.plot()

@@ -0,0 +1,3 @@
Clean up codes around signal.metadata
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Clean up codes around signal.metadata
Improve syntax in the `io` module.

hyperspy/io.py Outdated
Comment on lines 816 to 817
if "Markers" in signal.metadata:
del signal.metadata.Markers
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if "Markers" in signal.metadata:
del signal.metadata.Markers

What's about removing it completely? It is very very rare that we use del before an assignment in python and I don't see why it is necessary here. Looking at the history using git blame, it doesn't seem that there is a reason.

@ericpre ericpre merged commit 4a2f7ed into hyperspy:RELEASE_next_major Jan 30, 2023
@jlaehne jlaehne mentioned this pull request Mar 15, 2023
57 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants