Conversation
The file was using previous schemes and all the tests there had bugs. We need to add the new test file at some point
2d mean continuum jg
|
something weird is going on with the pylint checks |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new framework for computing delta fields through an ExpectedFlux module, specifically adding 2D mean continuum functionality that can compute the mean continuum as a function of both wavelength and redshift.
- Adds new
MeanContinuumInterpExpectedFluxclass with support for both 1D and 2D interpolation - Modifies the continuum computation utilities to handle 2D interpolators
- Updates configuration to accommodate new framework parameters
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| pylintrc | Extends line length limit and adds matrix variable names to accepted variables |
| py/picca/delta_extraction/expected_fluxes/utils.py | Updates continuum computation to support 2D interpolators and improves error handling |
| py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py | Implements new framework with 2D mean continuum computation capabilities |
| .github/workflows/pylint.yml | Pins pylint version for consistent CI behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py
Outdated
Show resolved
Hide resolved
py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py
Outdated
Show resolved
Hide resolved
py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py
Outdated
Show resolved
Hide resolved
py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py
Outdated
Show resolved
Hide resolved
py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py
Outdated
Show resolved
Hide resolved
py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py
Outdated
Show resolved
Hide resolved
py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py
Outdated
Show resolved
Hide resolved
py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py
Outdated
Show resolved
Hide resolved
py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py
Outdated
Show resolved
Hide resolved
|
Hi @iprafols thanks for the PR, did you include the changes done by Julien ? |
|
Did you find the issue with pylint ? |
|
Not yet, I can't seem to even reproduce the issue locally |
…p_expected_flux.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py
Outdated
Show resolved
Hide resolved
py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py
Outdated
Show resolved
Hide resolved
py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py
Outdated
Show resolved
Hide resolved
py/picca/delta_extraction/expected_fluxes/mean_continuum_interp_expected_flux.py
Outdated
Show resolved
Hide resolved
|
@corentinravoux can we merge this now? |
|
Hi @iprafols sorry I was not very available this week. |
There was a problem hiding this comment.
Those changes are fixes inside the main analysis ?
There was a problem hiding this comment.
The same code is used in the main analysis, yes. 1. There are three sets of changes:
- the one in lines 60-66 (new code) is a basic if to manage the new type for the mean_cont and does not have an impact on the main analyisis
- the one in lines 94-109 gets rid of a nasty "errors not positive defined" warning we kept having in iminuit. They don't seem to change the main analysis though
- the one in lines 119-120 is an extra test that the continuum is fine, and it might impact a few forests. Now intead of having them full of NaNs, they are simply ignored.
There was a problem hiding this comment.
In any case, none of the changes seem to impact the main analysis
corentinravoux
left a comment
There was a problem hiding this comment.
I just added two small questions but I am in favor for merging
|
Thanks for the changes, I agree that we can merge ! |
This PR implements a new framework to compute the delta field. This is encapsulated in a new ExpectedFlux module and should not affect the current standard runs
Wanted features that are still missing and will be added in future PRs