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

Deprecate setting metadata and original_metadata attributes #2913

Merged
merged 3 commits into from
Mar 30, 2022

Conversation

ericpre
Copy link
Member

@ericpre ericpre commented Mar 28, 2022

in favour of using set_item and add_dictionary methods or specifying metadata when creating signals - fix #2874 (comment).

Progress of the PR

  • Add deprecation warning when setting metadata and original_metadata attributes
  • [n/a] update docstring (if appropriate),
  • [n/a] 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

import hyperspy.api as hs
import numpy as np
s = hs.signals.Signal1D(np.arange(10))
s.original_metadata.add_dictionary({'a':'A', 'b':'B'})
print(s.original_metadata)

…our of using `set_item` and `add_dictionary` methods or specifying metadata when creating signals
@ericpre ericpre added this to the v1.7 milestone Mar 28, 2022
@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #2913 (ab87615) into RELEASE_next_minor (c7dc06c) will increase coverage by 0.25%.
The diff coverage is 93.18%.

❗ Current head ab87615 differs from pull request most recent head f06b534. Consider uploading reports for the commit f06b534 to get more accurate results

@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #2913      +/-   ##
======================================================
+ Coverage               79.58%   79.84%   +0.25%     
======================================================
  Files                     209      209              
  Lines                   32262    32558     +296     
  Branches                 7239     7328      +89     
======================================================
+ Hits                    25675    25995     +320     
+ Misses                   4836     4806      -30     
- Partials                 1751     1757       +6     
Impacted Files Coverage Δ
hyperspy/io_plugins/nexus.py 93.83% <ø> (-0.06%) ⬇️
hyperspy/misc/date_time_tools.py 86.95% <ø> (ø)
hyperspy/signal.py 76.20% <90.90%> (+0.10%) ⬆️
hyperspy/samfire.py 65.44% <91.66%> (+0.77%) ⬆️
hyperspy/_signals/complex_signal.py 93.75% <100.00%> (-0.04%) ⬇️
hyperspy/_signals/eds_sem.py 60.60% <100.00%> (ø)
hyperspy/_signals/eds_tem.py 83.50% <100.00%> (ø)
hyperspy/io.py 86.45% <100.00%> (ø)
hyperspy/misc/eels/eelsdb.py 65.21% <100.00%> (ø)
hyperspy/misc/utils.py 92.19% <100.00%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7dc06c...f06b534. Read the comment docs.

hyperspy/samfire.py Outdated Show resolved Hide resolved
hyperspy/signal.py Outdated Show resolved Hide resolved
hyperspy/signal.py Outdated Show resolved Hide resolved
hyperspy/signal.py Outdated Show resolved Hide resolved
hyperspy/signal.py Outdated Show resolved Hide resolved
hyperspy/samfire.py Outdated Show resolved Hide resolved
@jlaehne
Copy link
Contributor

jlaehne commented Mar 30, 2022

So the main idea is to prevent that people accidentally put a wrong data structure (dict instead of DictionaryTreeBrowser) into the s.metadata or s.original_metadata attributes of a signal?

I have not checked in detail yet, but I have the feeling that some tests or examples might still be using the direct access to the metadata tree.

@jlaehne jlaehne mentioned this pull request Mar 30, 2022
Copy link
Member

@jat255 jat255 left a comment

Choose a reason for hiding this comment

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

I haven't tested this manually, but it overall LGTM. I'm not familiar with the extension tests, so I'm not sure why those are failing.

@jat255
Copy link
Member

jat255 commented Mar 30, 2022

I have not checked in detail yet, but I have the feeling that some tests or examples might still be using the direct access to the metadata tree.

Yes, there are a number of these; here's one example: https://hyperspy.org/hyperspy-doc/current/user_guide/signal.html#:~:text=To%20change%20the%20default%20value%3A

We could either take care of this when the feature is actually removed (2.0), or now. I'm okay with either, although it's not a great user experience if a user follows an example in the docs and gets a warning as a result.

@hakonanes
Copy link
Contributor

I'm not familiar with the extension tests, so I'm not sure why those are failing.

pyxem developers are aware of tests failing (pyxem/pyxem#788 (comment)), and the failing kikuchipy tests are unrelated (pyxem/kikuchipy#509). Cannot speak for the single failing LumiSpy test.

@ericpre
Copy link
Member Author

ericpre commented Mar 30, 2022

So the main idea is to prevent that people accidentally put a wrong data structure (dict instead of DictionaryTreeBrowser) into the s.metadata or s.original_metadata attributes of a signal?

Yes, this is the idea, because setting the metadata to anything than an instance of DTB breaks many things, starting from the documentation!

I have not checked in detail yet, but I have the feeling that some tests or examples might still be using the direct access to the metadata tree.

I couldn't find any! :)

@ericpre
Copy link
Member Author

ericpre commented Mar 30, 2022

Yes, there are a number of these; here's one example: https://hyperspy.org/hyperspy-doc/current/user_guide/signal.html#:~:text=To%20change%20the%20default%20value%3A

@jat255, this is different: it is only setting a "leaf" of the DTB, not the metadata attribute itself.

Co-authored-by: Jonas Lähnemann <jonas@pdi-berlin.de>
Co-authored-by: Joshua Taillon <jat255@gmail.com>
@jlaehne
Copy link
Contributor

jlaehne commented Mar 30, 2022

I'm not familiar with the extension tests, so I'm not sure why those are failing.

pyxem developers are aware of tests failing (pyxem/pyxem#788 (comment)), and the failing kikuchipy tests are unrelated (pyxem/kikuchipy#509). Cannot speak for the single failing LumiSpy test.

The failing LumiSpy test is OK for the dev-version. So we just need to do our next release in time with v1.7.

@jlaehne
Copy link
Contributor

jlaehne commented Mar 30, 2022

Yes, there are a number of these; here's one example: https://hyperspy.org/hyperspy-doc/current/user_guide/signal.html#:~:text=To%20change%20the%20default%20value%3A

@jat255, this is different: it is only setting a "leaf" of the DTB, not the metadata attribute itself.

OK, setting a leaf will still be OK as direct assignment. I misunderstood that as well. Then I'm also happy with this PR as far as I can see.

@jlaehne jlaehne requested a review from jat255 March 30, 2022 21:12
@jlaehne jlaehne merged commit a1b206b into hyperspy:RELEASE_next_minor Mar 30, 2022
@ericpre ericpre deleted the metadata_setter branch March 31, 2022 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-extension-tests Run extension test suites type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants