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

Adopt RosettaSciIO's plugin design #694

Merged
merged 49 commits into from
Oct 29, 2024

Conversation

hakonanes
Copy link
Member

@hakonanes hakonanes commented Oct 27, 2024

Description of the change

This PR addresses the part of #650 concerning IO. It replaces #651.

The PR focuses on IO only, leaving updates of markers for another PR. Therefore, all tests related to markers are allowed to fail in this PR.

Progress of the PR

  • Docstrings for all functions
  • Unit tests with pytest for all lines
  • Clean code style by running black via pre-commit
  • IO tests pass
  • All other but marker tests pass
  • All docs apart from marker-related docs build as before
  • Explore simplifying support of H/ZSPY write plugins in our save() methods

For reviewers

  • The PR title is short, concise, and will make sense 1 year later.
  • New functions are imported in corresponding __init__.py.
  • New features, API changes, and deprecations are mentioned in the unreleased
    section in CHANGELOG.rst.
  • New contributors are added to kikuchipy/__init__.py and .zenodo.json.

Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
@hakonanes hakonanes added bug Something isn't working documentation This relates to the documentation maintenance This relates to package maintenance labels Oct 27, 2024
@hakonanes hakonanes added this to the v0.11.0 milestone Oct 27, 2024
@hakonanes hakonanes marked this pull request as draft October 27, 2024 10:58
@hakonanes hakonanes mentioned this pull request Oct 27, 2024
4 tasks
@ericpre
Copy link
Contributor

ericpre commented Oct 27, 2024

Okay, this is because of the way of the save method is overloaded. The changes still rely on private API, because rosettasciio API does have any public API on file format look-up, etc. As of today, it is expected to be used as follow:

from rsciio.hspy import file_writer
file_writer("beautifulplumage.hspy", fdict)

From a quick look, it seems that there are two formats that are supported for writting (nordif and kikuchipy_h5ebsd), so an alternative could be that you try to save to these based on the extension and otherwise fall back to BaseSignal.save? This has the benefit of not relying on private API and is simple.

Longer term, it would make sense to move these to rosettasciio, because the idea is to have the IO code in a standalone library and this will avoid the current issue.

@hakonanes
Copy link
Member Author

hakonanes commented Oct 27, 2024

Your solution sounds reasonable, I'll try that once all IO tests pass.

@hakonanes
Copy link
Member Author

The changes still rely on private API

I believe our IO depended on public HyperSpy <2 API. Which after moved to Rosetta became private. Not saying we should still use it, though.

Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
@hakonanes hakonanes marked this pull request as ready for review October 28, 2024 18:43
@hakonanes
Copy link
Member Author

an alternative could be that you try to save to these based on the extension and otherwise fall back to BaseSignal.save

Thanks for this suggestion, much cleaner solution. No explicit dependency on Rosetta needed either.

@hakonanes hakonanes merged commit b229aa8 into pyxem:hyperspy2-support Oct 29, 2024
3 of 10 checks passed
@hakonanes hakonanes deleted the hs2-io-compat branch October 29, 2024 18:38
@hakonanes
Copy link
Member Author

Not quite, a bit more work to get rid of the Rosetta dependency...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation This relates to the documentation maintenance This relates to package maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants