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

Add functions to search DictionaryTreeBrowser (e.g. metadata) #2633

Merged
merged 22 commits into from Feb 23, 2022

Conversation

jlaehne
Copy link
Contributor

@jlaehne jlaehne commented Jan 30, 2021

Description of the change

Add functions

  • to search if an item exists in DictionaryTreeBrowser (e.g. metadata),
  • to return its path,
  • to return its value
    The naming is derived from the has_item() and get_item() functions

Moved test_dictionary_tree_browser.py to misc folder.
Additionally, improved the test coverage of other functions in misc.utils.

Progress of the PR

  • Change implemented (can be split into several points),
  • integrate into existing methods has_item and get_item,
  • add handling parts of a path (from the back, i.e. item + preceding nodes),
  • update docstring (including some improvements to related docstrings),
  • update user guide (corrected typos in rest of file),
  • add entry to CHANGES.rst,
  • add tests,
  • increase coverage of misc.utils,
  • add lazy support,
  • ready for review.

Minimal example of the bug fix or the new feature

from hyperspy.misc.utils import DictionaryTreeBrowser
tree = DictionaryTreeBrowser({  "Node1": {"leaf11": 11,  "Node11": {"leaf111": 111},   })
tree.has_nested_item('leaf111')
tree.get_nested_path('leaf111') 
tree.get_nested_item('leaf111') 

Note that this example can be useful to update the user guide.

@codecov
Copy link

codecov bot commented Jan 30, 2021

Codecov Report

Merging #2633 (e0de507) into RELEASE_next_minor (32b501b) will increase coverage by 0.11%.
The diff coverage is 93.86%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #2633      +/-   ##
======================================================
+ Coverage               78.97%   79.09%   +0.11%     
======================================================
  Files                     206      206              
  Lines                   31868    31907      +39     
  Branches                 7161     7181      +20     
======================================================
+ Hits                    25169    25238      +69     
+ Misses                   4939     4910      -29     
+ Partials                 1760     1759       -1     
Impacted Files Coverage Δ
hyperspy/misc/test_utils.py 88.13% <70.00%> (ø)
hyperspy/misc/utils.py 91.94% <95.42%> (+6.84%) ⬆️
hyperspy/_signals/lazy.py 92.50% <0.00%> (-0.32%) ⬇️
hyperspy/drawing/utils.py 74.51% <0.00%> (-0.30%) ⬇️
hyperspy/io_plugins/phenom.py 85.38% <0.00%> (-0.23%) ⬇️

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 32b501b...e0de507. Read the comment docs.

@jlaehne
Copy link
Contributor Author

jlaehne commented Mar 4, 2021

How do I add labels to a PR @ericpre @thomasaarholt , do I need extra permissions?

@ericpre
Copy link
Member

ericpre commented Mar 13, 2021

This would be useful functionality to have but I am not sure if this is a good time to add more functionality to DictionaryTreeBrowser. As discussed in #2623 (comment), we should consider more standard alternative for hyperspy 2.0: the DTB have been very useful for more than 10 years now but it may be time time to switch to something better!

What do you think?

@jlaehne
Copy link
Contributor Author

jlaehne commented Mar 15, 2021

I've seen #2623 (comment) - but it came only after implementing this one. Couldn't we integrate this functionality anyway at the moment. Should not be a problem to reproduce in the new framework once we move to a new metadata implementation. As far as I can see, neither https://github.com/mewwts/addict nor https://github.com/Infinidat/munch have search capabilities, so it would have to either be contributed there or again implemented on the HyperSpy side.

@jlaehne
Copy link
Contributor Author

jlaehne commented May 2, 2021

Made this PR compatible with the new lazy DTB. Note that as tests were missing, also export was not working with lazy.

@ericpre
Copy link
Member

ericpre commented May 3, 2021

>>> from hyperspy.misc.utils import DictionaryTreeBrowser
>>> tree = DictionaryTreeBrowser(
    {
        "Node1": {"leaf11": 11,
                  "Node11": {"leaf111": 111},
                  },
        "Node2": {"leaf21": 21,
                  "Node21": {"leaf211": 211},
                  },
    })

>>> tree.get_nested_item('Node1.Node11')

>>> tree.get_item('Node1.Node11')
    └── leaf111 = 111

In the case above, is it intended that tree.get_nested_item('Node1.Node11') returns None?

From the API perspective, would it make simple to add this functionality to get_item and has_item with a keyword argument? For example, it could something along the lines of full_path=True to keep the existing behaviour of get_item and for full_path=False, it will return the same output as the get_nested_item of this PR.

@jlaehne
Copy link
Contributor Author

jlaehne commented May 3, 2021

In the case above, is it intended that tree.get_nested_item('Node1.Node11') returns None?

Well, the idea I had in mind was to look for a specific key and therefore looking for a chain of keys does not work. tree.get_nested_item('Node11') would work.

From the API perspective, would it make simple to add this functionality to get_item and has_item with a keyword argument? For example, it could something along the lines of full_path=True to keep the existing behavior of get_item and for full_path=False, it will return the same output as the get_nested_item of this PR.

Would be a possibility. In fact, could even decide automatically based on the length of item_path which way to go. I just did not want to touch the existing function at first.

@jlaehne jlaehne added this to the v1.7 milestone Feb 17, 2022
@jlaehne
Copy link
Contributor Author

jlaehne commented Feb 17, 2022

>>> from hyperspy.misc.utils import DictionaryTreeBrowser
>>> tree = DictionaryTreeBrowser(
    {
        "Node1": {"leaf11": 11,
                  "Node11": {"leaf111": 111},
                  },
        "Node2": {"leaf21": 21,
                  "Node21": {"leaf211": 211},
                  },
    })

>>> tree.get_nested_item('Node1.Node11')

>>> tree.get_item('Node1.Node11')
    └── leaf111 = 111

In the case above, is it intended that tree.get_nested_item('Node1.Node11') returns None?

Remains to be done, but could be a useful feature as we are at it.

From the API perspective, would it make simple to add this functionality to get_item and has_item with a keyword argument? For example, it could something along the lines of full_path=True to keep the existing behaviour of get_item and for full_path=False, it will return the same output as the get_nested_item of this PR.

Implemented like proposed here.

@jlaehne
Copy link
Contributor Author

jlaehne commented Feb 21, 2022

In the case above, is it intended that tree.get_nested_item('Node1.Node11') returns None?

Remains to be done, but could be a useful feature as we are at it.

Partial paths (from the item-key backwards) work as well now.

@ericpre could you give this PR another review?

@ericpre ericpre merged commit 2856c92 into hyperspy:RELEASE_next_minor Feb 23, 2022
@jlaehne jlaehne deleted the get_nested branch February 23, 2022 21:21
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.

None yet

2 participants