h2020charisma / ramanchada2

A library for Raman spectroscopy harmonization
https://h2020charisma.github.io/ramanchada2/
MIT License
4 stars 3 forks source link

Migrate to numpy 2 #128

Closed georgievgeorgi closed 2 months ago

georgievgeorgi commented 3 months ago
github-actions[bot] commented 3 months ago

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/ramanchada2/misc/utils
  argmin2d.py 67
  src/ramanchada2/spectrum/baseline
  add_baseline.py
  src/ramanchada2/spectrum/filters
  add_gaussian_noise.py
  add_gaussian_noise_drift.py
  add_poisson_noise.py
  tests/misc
  test_argmin2d_align.py
  tests/spectrum
  test_random_generator_seeds.py
Project Total  

This report was generated by python-coverage-comment-action

georgievgeorgi commented 3 months ago

TODO: make sure it works with numpy 1

vedina commented 3 months ago

please check if it works with both numpy<2 & numpy>=2

georgievgeorgi commented 3 months ago

All the tests pass successfully with both numpy<2 and numpy>=2. So the PR seems safe to be merged.

kerberizer commented 3 months ago

@georgievgeorgi Thank you very much for the work on this! Do you think it is realistic to cover with tests the few remaining statements per the coverage report above?

georgievgeorgi commented 3 months ago

In the tests i found that i doubled the tests for add_gaussian_noise and missed the add_poisson_noise (@kerberizer, thank you for drawing my attention there). Currently i am playing with tests for argmin2d.

georgievgeorgi commented 2 months ago

@kerberizer, could you please review and merge this pull request

kerberizer commented 2 months ago

@georgievgeorgi I was under the impression that you wanted to add more tests. Are we going to aim for 100% test coverage? It's one single statement, but still.

georgievgeorgi commented 2 months ago

I understand. I fixed a copy-paste issue and added an extra test which covers argmin2d. Adding a test for align_shift, however, is not that straight forward. With the provided tests the code coverage is improved anyway and as the main purpose of the pull request is not to provide tests but to fix the numpy2 issues, i think it can be merged.