Change unit test response file to dense

[israelmcmc]

We have moved from a sparse to a dense response. It was not that sparse, and it ended up being slower than a dense matrix. The unit test now reflects what we actually use.

I created the dense response with:

from cosipy.response import FullDetectorResponse
from cosipy import test_data
from histpy import Histogram

dr = FullDetectorResponse.open(test_data.path/'test_full_detector_response.h5')

dr_new = Histogram.concatenate(dr.axes['NuLambda'], list(dr))
dr_new = dr_new.project(['Ei', 'NuLambda', 'Em', 'Phi', 'PsiChi'])
dr_new = dr_new.todense()

FullDetectorResponse._write_h5(dr_new, test_data.path/'test_full_detector_response_dense.h5')

In order to make things easier I moved the code that writes an HDF5 response file from a Histogram from _open_rsp to its own function, _write_h5. I added a unit test for this specific function.

[codecov]

Codecov Report

Attention: Patch coverage is 34.48276% with 19 lines in your changes missing coverage. Please review.

Project coverage is 80.41%. Comparing base (4b2700e) to head (8f1a689).
Report is 25 commits behind head on v0.3.x.

Files with missing lines	Patch %	Lines
cosipy/response/FullDetectorResponse.py	34.48%	19 Missing ⚠️

❌ Your patch status has failed because the patch coverage (34.48%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Files with missing lines	Coverage Δ
cosipy/response/FullDetectorResponse.py	55.41% <34.48%> (+5.97%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[israelmcmc]

I need to create a fake .rsp to fix the test coverage. @augustus-thomas I think at some point you were working on this, did you get to create one? Or maybe it was for the PSR, I don't remember exactly.

[hiyoneda]

maybe, should it be tmp_path.path / 'tmp_rsp.h5'? Currently, the file will be saved in the test data directory.

[israelmcmc]

Thanks for the catch. Indeed it was the wrong path. I fixed it in my latest commit.

[ckarwin]

@israelmcmc I will try to check this tomorrow. I think it will help give me better coverage for my PR #300. Do 100% of new lines really need to be covered by unit tests for a new PR to pass the coverage requirement?

[israelmcmc]

Thanks @ckarwin. Yeah, I think it's related to the covered code in #300.

And nope, there's a 100% target with, currently, a 50% leeway ;)
https://github.com/cositools/cosipy/blob/develop/codecov.yml

[ckarwin]

Ah ok. I think the problem is that my current coverage is 0%, even though it's just four lines. After merging your PR I think it will be above 50%.

[ckarwin]

@israelmcmc

I think the new _write_h5 method will remove the fix that I made for reading the polarization response in PR #300. The problem is the axes definition here: https://github.com/israelmcmc/cosipy/blob/8b4850b66da9dd7332b03c3e7520f5363ddf61b0/cosipy/response/FullDetectorResponse.py#L679

For a polarization response it should be [1, 0, 2, 3, 4, 5]. In PR #300 I fixed this by adding an if statement that checks if the response has polarization.

Can you check this please?

[israelmcmc]

@ckarwin Yeah, good catch. I synced it with the latest version of v0.3.x and resolved a bunch of conflicts, including the one you mentioned (and resolved it to keep your changes).

I also updated the PR to be merged with v0.3.x. I'll merge it with develop afterwards.

[ckarwin]

Thanks for making the changes. I tested your latest code by opening one of the DC3 response rsp files, and it works fine.

One more thing: I see the unit test now uses the dense response. Do you want to also repeat the test for the sparse response? Or do you prefer not to since it will require additional time? I was thinking that it would help the code coverage exceed 50%. Otherwise, we can override this requirement.

[israelmcmc]

Cool! Thanks for checking.

I was considering removing the sparse response altogether. It seems like we'll no longer use it, and it's just one more piece of code we have to maintain. What do you think?

As for the code coverage, the main piece of code that is not being exercised by the test suite is opening an .rsp file and converting it to HDF5. The test I modified opens the HDF5. _write_h5 was a colateral, since I needed it to create the test HDF5 response.

[ckarwin]

I was considering removing the sparse response altogether. It seems like we'll no longer use it, and it's just one more piece of code we have to maintain. What do you think?

Hmm, maybe eventually, but I would leave it for now. I think it doesn't hurt to keep, and it could be useful at some point.

As for the code coverage, the main piece of code that is not being exercised by the test suite is opening an .rsp file and converting it to HDF5.

Ah yes, that's right. We should create a dummy rsp test response at some point. I used this method a lot for converting the DC3 response files from Andreas to h5.

I'm ok to merge this now if you're ready.

[ckarwin]

@israelmcmc Did you want me to hold off on merging this?

[israelmcmc]

@ckarwin Yeah, I don't have a strong opinion, but since this is not needed for DC3, I thought it would be better to wait a little to see if I can add tests to cover my new code.

[ckarwin]

I see. No problem. Yeah let's wait.

[ckarwin]

This PR is needed for PR#314 (@Yong2Sheng , @GallegoSav), and so we decided to go ahead and merge.

[ckarwin]

Fine to be merged.

[ckarwin]

@israelmcmc I approved this but I guess I can't push to this branch.

[israelmcmc]

Thanks @ckarwin. I think the merge button is blocked because of the code coverage check. I'll bypass it manually.

I'll merge it with v0.3.x, but I'll wait until I test all tutorials to make a new release. Everything in v0.3.x is now (before this PR merge) in the 0.3.1 release.