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

Estimation syntax consistency #9

Merged
merged 2 commits into from
Oct 26, 2023
Merged

Estimation syntax consistency #9

merged 2 commits into from
Oct 26, 2023

Conversation

aimalz
Copy link
Contributor

@aimalz aimalz commented Aug 14, 2023

Apologies, I seem to have failed to make the PR for this one when I made the others in the omnibus rail.estimation renaming. Note that the checks were failing before work on this began due to a thing that will be fixed by #7.

@aimalz
Copy link
Contributor Author

aimalz commented Sep 26, 2023

@sschmidt23 I could use some help fixing the bug that's causing the tests to fail. I don't think it's related to the changes from this PR, but it would be good to merge this before LSSTDESC/rail#73 goes through. (Also tagging @OliviaLynn for completeness.)

@sschmidt23
Copy link
Collaborator

I think it's because hdf5_groupname was added to SHARED_PARAMS with a default value of "photometry", so it gets assigned now even if not specified, which was not the case when the original test was written.

@aimalz aimalz changed the title User/aimalz/renaming Estimation syntax consistency Oct 3, 2023
@aimalz aimalz added bug Something isn't working help wanted Extra attention is needed labels Oct 18, 2023
@eacharles eacharles force-pushed the user/aimalz/renaming branch from 726a259 to 08fd581 Compare October 26, 2023 02:02
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d2adcd9) 100.00% compared to head (08fd581) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main        #9   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          147       147           
=========================================
  Hits           147       147           
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/rail/estimation/algos/cmnn.py 100.00% <100.00%> (ø)

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

@sschmidt23 sschmidt23 self-requested a review October 26, 2023 02:06
Copy link
Collaborator

@sschmidt23 sschmidt23 left a comment

Choose a reason for hiding this comment

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

fixed now, approved

@sschmidt23 sschmidt23 merged commit c0195c4 into main Oct 26, 2023
@sschmidt23 sschmidt23 deleted the user/aimalz/renaming branch October 26, 2023 02:07
@eacharles
Copy link
Collaborator

closed with #9

@aimalz
Copy link
Contributor Author

aimalz commented Oct 30, 2023

Thanks again @sschmidt23 for taking care of this while I was out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants