Skip to content

Set correct signal type of Hamamatsu tiff images, if lumispy is installed#386

Merged
ericpre merged 3 commits into
hyperspy:mainfrom
jlaehne:tiff-lumispy
Apr 9, 2025
Merged

Set correct signal type of Hamamatsu tiff images, if lumispy is installed#386
ericpre merged 3 commits into
hyperspy:mainfrom
jlaehne:tiff-lumispy

Conversation

@jlaehne

@jlaehne jlaehne commented Apr 5, 2025

Copy link
Copy Markdown
Member

Set signal_type of Hamamatsu streak images loaded from tiff format to TransientSpectrum (LumiTransientSpectrum class), if lumispy is installed.

Add tests to tiff and hamamatsu readers that run when lumispy is available.

Progress of the PR

  • Change implemented (can be split into several points),
  • 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)
  • add tests,
  • ready for review.

@azure-pipelines

Copy link
Copy Markdown
There was an error handling pipeline event 4c4b94c0-44df-427b-a073-de66fb4b7e03.

@codecov

codecov Bot commented Apr 5, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.90%. Comparing base (c53293a) to head (238e3d6).
Report is 112 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #386   +/-   ##
=======================================
  Coverage   87.90%   87.90%           
=======================================
  Files          86       86           
  Lines       11269    11271    +2     
  Branches     2088     2088           
=======================================
+ Hits         9906     9908    +2     
  Misses        864      864           
  Partials      499      499           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread rsciio/hamamatsu/_api.py Outdated
signal["signal_type"] = ""
else:
signal["signal_type"] = "LumiTransientSpectrum" # pragma: no cover
signal["signal_type"] = "LumiTransientSpectrum"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be worth adding a build on GitHub CI that install extensions (lumispy, pyxem, etc.) to test this kind of thing?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Possibly. That is what the integration tests on the hyperspy-extensions-list repro are doing - but then it is not noticed as directly if something goes wrong. I can also add back the pragma: no cover and we leave it to the integration tests to run this part?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That is what the integration tests on the hyperspy-extensions-list repro are doing - but then it is not noticed as directly if something goes wrong.

Indeed, but this can be tedious when fixing it or figure out what needs to be fixed. We could use the same approach as in hyperspy with a separate workflow that runs on demand (https://github.com/hyperspy/hyperspy/blob/RELEASE_next_minor/.github/workflows/tests_extension.yml) but we trend to forget about it and also it doesn't help with the coverage.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Adding such tests in #388

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Regarding the coverage, should we add back # pragma: no cover, to silent coverage warning and add a comment that this is tested in the integration test workflow but the coverage is not reported?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

@jlaehne jlaehne force-pushed the tiff-lumispy branch 4 times, most recently from 5f21245 to 31f2b3b Compare April 6, 2025 10:11
@ericpre ericpre merged commit 01eb721 into hyperspy:main Apr 9, 2025
@ericpre ericpre added this to the v0.8.1 milestone Apr 9, 2025
@ericpre ericpre modified the milestones: v0.8.1, v0.9 May 28, 2025
@jlaehne jlaehne deleted the tiff-lumispy branch July 26, 2025 10:25
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