LSSTDESC / rail_lephare

RAIL wrapper to the LePHARE photometric redshift code
MIT License
1 stars 0 forks source link

Update for ceci version 2 #53

Closed joezuntz closed 3 months ago

joezuntz commented 3 months ago

This PR updates the constructors of all stage subclasses to work with ceci version 2, in which aliases are specified in the constructor. A similar PR has been opened for each RAIL repo.

The main change is to the the constructors to accept any keywords with **kwargs and pass them to the parent class.

I have also removed any constructors which did not do anything except call the parent class constructor. Since this happens automatically if you omit the constructor, these were redundant.

Finally, in constructors I have changed I have removed hard-coded parent classes in favour of the python3 recommended super() function. If the parent class are changed in the future the constructor will still work.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 46.57%. Comparing base (3f512c3) to head (9769526).

Files Patch % Lines
src/rail/estimation/algos/lephare.py 75.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #53 +/- ## ======================================= Coverage 46.57% 46.57% ======================================= Files 2 2 Lines 146 146 ======================================= Hits 68 68 Misses 78 78 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

joezuntz commented 3 months ago

Hi all - this is ready to review now - see my note on slack #desc-pz-rail for a plan, should not merge it yet.

raphaelshirley commented 3 months ago

Changes look fine. I will make some tests with the new code to check it all runs through our examples...

joezuntz commented 3 months ago

Am I allowed to merge this or should one of the owners do it?

hangqianjun commented 3 months ago

Am I allowed to merge this or should one of the owners do it?

I didn't merge as I saw @raphaelshirley's comment on testing this with the new code. @raphaelshirley are you happy with this to be merged?

raphaelshirley commented 3 months ago

Hi yes this can be merged. Tests ran locally.