Skip to content

Conversation

@avalluvan
Copy link
Contributor

The recently merged pull request on RL Parallelization v2 #408 did not run out-of-the-box upon cosipy installation. The issue can be traced back to the update from histpy 1.x to 2.x. In the former, the underflow and overflow bins were tracked by default which required some workarounds for the RL dataIF code. As this tracking feature has been set to False by default in histpy 2.x, the workarounds had to be removed. This pull request verifies the code so that it can work with histpy 2.x.

Changes made:

  • RLparallelscript.py: Remove *_DIR and update "- 2" in nrows, ncols
  • dataIF_Parallel.py: Update response loader function to work with histpy 2.0

…ot track under/over flow bins by default)

Remove comments which were relevant to histpy 1.x
@avalluvan
Copy link
Contributor Author

Aside, I also verified that this code works with the MAP_RL implementation. The serial and parallel results look identical.
With parallel code,
image
With serial code,
image

@codecov
Copy link

codecov bot commented Oct 21, 2025

Codecov Report

❌ Patch coverage is 11.76471% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.03%. Comparing base (f22aa8a) to head (9c4df5b).
⚠️ Report is 97 commits behind head on develop.

Files with missing lines Patch % Lines
cosipy/image_deconvolution/dataIF_Parallel.py 11.76% 15 Missing ⚠️
Files with missing lines Coverage Δ
cosipy/image_deconvolution/RLparallelscript.py 0.00% <ø> (ø)
cosipy/image_deconvolution/dataIF_Parallel.py 14.43% <11.76%> (-0.71%) ⬇️

... and 3 files with indirect coverage changes

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

@hiyoneda
Copy link
Contributor

Thanks, @avalluvan. Can we check if the input histogram has overflow/underflow bins first, and then adjust the way we slice it?

@hiyoneda
Copy link
Contributor

For a pre-computed extended source response, you can check the overflow bins like this.

from cosipy.response import ExtendedSourceResponse

rsp = ExtendedSourceResponse.open('511_gal_DC2_earthocc_nooverflow.h5')

rsp.track_overflow() # it returns like array([False, False, False, False, False]) for each axis ('NuLambda', 'Ei', 'Em', 'Phi', 'PsiChi').

@avalluvan
Copy link
Contributor Author

@hiyoneda the recommended changes have been incorporated

@hiyoneda
Copy link
Contributor

Thanks, Anaya. It is ready to be merged.

@hiyoneda
Copy link
Contributor

Do you know how we can pass the codecov/path check for the parallel RL?

@avalluvan
Copy link
Contributor Author

Hi Hiroki. Gentle follow up to approve the pull request and complete the merge.

@hiyoneda
Copy link
Contributor

hiyoneda commented Oct 31, 2025

Yes, I can approve it, but the codecov/path looks like it is failing, so the merging is blocked. I think that the unittest should cover dataIF_parallel, but it might be difficult due to OpenMPI. Do you remember how you solved it before (maybe @israelmcmc)?

@israelmcmc
Copy link
Collaborator

It's an open issu (#409). For now I think the only way is to run pytest-cov locally in a system that has MPI.

@avalluvan Please add the necessary tests and check the coverage with pytest-cov. I'll take your word for it and bypass the codecov enforcement. Here are the instructions: https://github.com/cositools/cosipy/blob/develop/docs/dev/unit_tests.rst

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants