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

Fix test suite without optional dependencies #182

Merged
merged 5 commits into from
Nov 13, 2023

Conversation

ericpre
Copy link
Member

@ericpre ericpre commented Nov 10, 2023

I am not sure why we didn't caught it earlier but I suspect that that installing hyperspy dev will pull these dependencies...

Progress of the PR

  • read/write EMD NCEM file without numba/sparse dependencies,
  • Fix jit_if_numba in tvips reader,
  • Fix JEOL reader when numba is not installed,
  • [n/a] update docstring (if appropriate),
  • update user guide (if appropriate),
  • add a changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
  • Check formatting of the changelog entry (and eventual user guide changes) in the docs/readthedocs.org:rosettasciio build of this PR (link in github checks)
  • [n/a] add tests,
  • ready for review.

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Attention: 86 lines in your changes are missing coverage. Please review.

Comparison is base (cdc8cfa) 85.93% compared to head (b665071) 86.02%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #182      +/-   ##
==========================================
+ Coverage   85.93%   86.02%   +0.08%     
==========================================
  Files          79       81       +2     
  Lines       10365    10387      +22     
  Branches     2253     2253              
==========================================
+ Hits         8907     8935      +28     
+ Misses        936      931       -5     
+ Partials      522      521       -1     
Files Coverage Δ
rsciio/jeol/_api.py 97.45% <100.00%> (+0.01%) ⬆️
rsciio/tvips/_api.py 98.43% <100.00%> (ø)
rsciio/utils/hdf5.py 97.72% <100.00%> (+0.58%) ⬆️
rsciio/utils/tools.py 80.47% <100.00%> (+0.38%) ⬆️
rsciio/emd/_api.py 77.19% <77.77%> (-8.09%) ⬇️
rsciio/emd/_emd_ncem.py 92.11% <92.11%> (ø)
rsciio/emd/_emd_velox.py 84.02% <84.02%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rsciio/emd/_emd_ncem.py Fixed Show fixed Hide fixed
rsciio/emd/_emd_ncem.py Fixed Show fixed Hide fixed
rsciio/emd/_emd_velox.py Fixed Show fixed Hide fixed
rsciio/tests/test_emd_ncem.py Fixed Show fixed Hide fixed
rsciio/tests/test_emd_ncem.py Fixed Show fixed Hide fixed
rsciio/tests/test_emd_ncem.py Fixed Show fixed Hide fixed
rsciio/tests/test_emd_ncem.py Fixed Show fixed Hide fixed
rsciio/emd/_emd_velox.py Fixed Show fixed Hide fixed
rsciio/emd/_emd_ncem.py Fixed Show fixed Hide fixed
rsciio/emd/_emd_velox.py Fixed Show fixed Hide fixed
@ericpre ericpre force-pushed the fix_sparse_numba_optional_test_suite branch from 8a3f8d3 to b3325e8 Compare November 12, 2023 10:35
@ericpre ericpre force-pushed the fix_sparse_numba_optional_test_suite branch from b3325e8 to bc6d9ad Compare November 12, 2023 11:55
@CSSFrancis
Copy link
Member

@ericpre I can look at this but just to make sure that I understand are there any changes to the emd readers or are they just split?

@ericpre
Copy link
Member Author

ericpre commented Nov 13, 2023

Yes, there are just split for the following reasons:

  1. Two files are always better to read than a single large (>1600 lines!) file.
  2. Allow delaying import to when it is actually necessary, which avoid importing numba/sparse in some cases.

@@ -25,6 +25,7 @@
import pytest

hs = pytest.importorskip("hyperspy.api", reason="hyperspy not installed")
pytest.importorskip("h5py", reason="hyperspy not installed")
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
pytest.importorskip("h5py", reason="hyperspy not installed")
pytest.importorskip("h5py", reason="h5py not installed")

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, I simplified it by removing the reason argument as it is not necessary.

@CSSFrancis
Copy link
Member

@ericpre This all seems good, but as for this not being caught it might be good to add a CI which only tests the minimal install for RosettaSciIO?

@ericpre
Copy link
Member Author

ericpre commented Nov 13, 2023

My first comment on not being caught was misleading it wasn't initially, when we merged the PR make some of these optional, but It is now being caught on CI: see the last commit on the main branch: https://github.com/hyperspy/rosettasciio/actions/workflows/tests.yml?query=branch%3Amain

@CSSFrancis
Copy link
Member

My first comment on not being caught was misleading it wasn't initially, when we merged the PR make some of these optional, but It is now being caught on CI: see the last commit on the main branch: https://github.com/hyperspy/rosettasciio/actions/workflows/tests.yml?query=branch%3Amain

Okay sounds good then! LGTM You can merge it if you would like

@ericpre ericpre merged commit 948b9e2 into hyperspy:main Nov 13, 2023
31 checks passed
@ericpre ericpre added this to the v0.3 milestone Nov 14, 2023
@ericpre ericpre mentioned this pull request Nov 25, 2023
6 tasks
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.

2 participants