cositools / cosipy

The COSI high-level data analysis tools
Apache License 2.0
3 stars 16 forks source link

Cleanup and tests #175

Closed israelmcmc closed 4 weeks ago

israelmcmc commented 1 month ago

I'm reorganizing the cosipy module in preparation for the unit tests. Each module will have at least a test.

Work done so far:

Fixes #170

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 27.46%. Comparing base (d7924b8) to head (1222229). Report is 14 commits behind head on develop.

Files Coverage Δ
cosipy/__init__.py 100.00% <ø> (+100.00%) :arrow_up:
cosipy/_version.py 100.00% <ø> (ø)
cosipy/data_io/BinnedData.py 13.55% <100.00%> (+13.55%) :arrow_up:
cosipy/data_io/DataIO.py 100.00% <100.00%> (+100.00%) :arrow_up:
cosipy/data_io/ReadTraTest.py 16.04% <ø> (+16.04%) :arrow_up:
cosipy/data_io/make_plots.py 13.63% <ø> (ø)
...mage_deconvolution/deconvolution_algorithm_base.py 19.81% <ø> (+19.81%) :arrow_up:
cosipy/image_deconvolution/image_deconvolution.py 28.16% <100.00%> (+28.16%) :arrow_up:
cosipy/image_deconvolution/modelmap.py 37.50% <ø> (+37.50%) :arrow_up:
cosipy/response/FullDetectorResponse.py 39.61% <100.00%> (+39.61%) :arrow_up:
... and 1 more

... and 22 files with indirect coverage changes

ckarwin commented 1 month ago

@israelmcmc I started adding more unit tests to dataIO, and I'm working from this branch. I noticed one small thing: the unit test for data fetching (test_data_fetching.py) writes the test file (test_file.txt) to the root directory (cosipy). Probably we want to remove the file at the end of the test script.

israelmcmc commented 1 month ago

@ckarwin Yes, good catch. I fixed it by using tempfile to create a system temporary folder that is automatically removed at the end of the tests. Since I was at it, I added a few test cases to have full coverage for that file.

However, due to what it seems an unrelated issue, the automatic pipelines are not failing. I think it has to do with new releases of some dependencies. I'll look into it, but they are passing on my machine.

ckarwin commented 1 month ago

@israelmcmc Yeah, everything is still passing on my machine as well, after pulling your latest commit.

ckarwin commented 1 month ago

@Yong2Sheng @israelmcmc Thanks, Yong, for approving the PR. So can it be merged soon, or still waiting on something? I see there is still a problem with some of the unit tests.

israelmcmc commented 1 month ago

Thanks @Yong2Sheng and @ckarwin. I'd like to have the test passing before merging. I think it's a problem with the dependencies, maybe I just need to specify the versions. I'll work on it.

ckarwin commented 1 month ago

Thanks @israelmcmc! Yeah, it would be helpful to have it merged whenever you can, to make it easier for adding more tests.

israelmcmc commented 1 month ago

In the meantime, people can branch off my branch (israelmcmc:cleanup_and_tests) instead of develop, and everything else is the same. But yet, I agree it's easier and better to just merge it, I'll get to it

Yong2Sheng commented 1 month ago

Hi @israelmcmc, do you mean to fork and make PR to your branch (israelmcmc:cleanup_and_test)? Thanks!

israelmcmc commented 1 month ago

@Yong2Sheng Kind off. Yes to making a new branch from my branch, but you don't need to fork my repo and you can open the PR directly to develop on this repo.

ckarwin commented 1 month ago

@israelmcmc I tested the error in the checks as follows:

  1. I Copied your cleanup and tests branch into my forked repo.
  2. I Made a PR #185, to replicate the error that you get.
  3. Indeed, I get the same errors related to matplotlib.cm, relating to the discussion in issue #180.
  4. I then updated install_requires in setup.py file with 'matplotlib<=3.8.3'. After doing this all checks passed.

So I think if you do the same in your setup file it should fix the issue. Can you try?

ckarwin commented 1 month ago

@israelmcmc Actually, I just made a PR into your branch with the update, if that's easier.

israelmcmc commented 4 weeks ago

Thanks, @ckarwin. That worked! Please go ahead and approve and merge this.

ckarwin commented 4 weeks ago

Excellent.