Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update for ceci version 2 #53

Merged
merged 6 commits into from
Jul 29, 2024
Merged

Update for ceci version 2 #53

merged 6 commits into from
Jul 29, 2024

Conversation

joezuntz
Copy link
Contributor

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.

Copy link

codecov bot commented Jul 11, 2024

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 ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #53   +/-   ##
=======================================
  Coverage   46.57%   46.57%           
=======================================
  Files           2        2           
  Lines         146      146           
=======================================
  Hits           68       68           
  Misses         78       78           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joezuntz
Copy link
Contributor Author

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
Copy link
Collaborator

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

Copy link
Contributor

@hangqianjun hangqianjun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with the new changes!

@joezuntz
Copy link
Contributor Author

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

@hangqianjun
Copy link
Contributor

hangqianjun commented Jul 29, 2024

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
Copy link
Collaborator

Hi yes this can be merged. Tests ran locally.

Copy link
Collaborator

@raphaelshirley raphaelshirley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks.

@hangqianjun hangqianjun merged commit 55ce8ce into main Jul 29, 2024
6 checks passed
@hangqianjun hangqianjun deleted the ceci2 branch July 29, 2024 09:19
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