-
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
EMD Velox: update error msg for missing sparse module #305
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #305 +/- ##
==========================================
- Coverage 87.67% 87.66% -0.02%
==========================================
Files 83 83
Lines 11149 11152 +3
Branches 2414 2414
==========================================
+ Hits 9775 9776 +1
- Misses 860 862 +2
Partials 514 514 ☔ View full report in Codecov by Sentry. |
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 @dbdurham for the pull request, this looks good. Can you try to add a test in https://github.com/hyperspy/rosettasciio/blob/main/rsciio/tests/test_emd_velox.py to cover the code that you added? It will need to catch the error using pytest.raises
but also update the pytest.importorskip("sparse")
accordingly to enable running the test.
I am happy to do that if this helps.
Can you add a changelog entry? The instruction on how to do that are in https://github.com/hyperspy/rosettasciio/blob/main/upcoming_changes/README.rst.
@ericpre sure I will try that, thanks for your suggestions as this is my
first time contributing to a large project like this. May take me a few
days to get to the tests and get it right if that's ok, as it's also my
first time working with pytest.
…On Wed, Aug 28, 2024 at 1:18 PM Eric Prestat ***@***.***> wrote:
***@***.**** commented on this pull request.
Thank you @dbdurham <https://github.com/dbdurham> for the pull request,
this looks good. Can you try to add a test in
https://github.com/hyperspy/rosettasciio/blob/main/rsciio/tests/test_emd_velox.py
to cover the code that you added? It will need to catch the error using
pytest.raises but also update the pytest.importorskip("sparse")
accordingly to enable running the test.
I am happy to do that if this helps.
Can you add a changelog entry? The instruction on how to do that are in
https://github.com/hyperspy/rosettasciio/blob/main/upcoming_changes/README.rst
.
—
Reply to this email directly, view it on GitHub
<#305 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMSHAAHWFCMNEDLT2IGMVY3ZTYH7LAVCNFSM6AAAAABNEVMG5CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENRWHE3TCMBWG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@dbdurham, are you still planning to do the changes? |
@ericpre I will no longer be able to complete these changes, sorry for the
long delay
…On Mon, Dec 2, 2024 at 11:35 AM Eric Prestat ***@***.***> wrote:
@dbdurham <https://github.com/dbdurham>, are you still planning to do the
changes?
—
Reply to this email directly, view it on GitHub
<#305 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMSHAABF5X4TZKRTIBD3EUT2DSK6FAVCNFSM6AAAAABNEVMG5CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJSGI2DONRTHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Description of the change
This is an update in response to Issue #296 "emd reader Velox ModuleNotFoundError." When attempting to load EDS Spectrum Image data from a Velox EMD file without the optional "sparse" dependency installed, a ModuleNotFoundError is raised which is confusing to some users. To clarify the issue, a try except statement was added around running the Velox EMD file reader and a new error message is printed.
Progress of the PR
upcoming_changes
folder (seeupcoming_changes/README.rst
),docs/readthedocs.org:rosettasciio
build of this PR (link in github checks)