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

Move in bpz notebooks, update paths; copy over cmnn nb changes #109

Merged
merged 3 commits into from
Nov 21, 2023

Conversation

OliviaLynn
Copy link
Member

@OliviaLynn OliviaLynn commented Nov 14, 2023

Problem & Solution Description (including issue #)

  • Moving in bpz notebooks for Gather demo notebooks from older standalones #48
  • Paths have been adjusted as needed
  • I wasn't 100% sure on the other two data files that were stored next to the notebooks (are they also meant to be kept separate like the tar?), so I added them to the rail get-data --bpz-demo-data script in rail_base#76.

Also copies over updates to CMNN notebook made in CMNN repo to the notebook here

That was on me, should have deleted it sooner. PR is out to remove it at LSSTDESC/rail_cmnn#13

Code Quality

  • My code follows the code style of this project
  • I have written unit tests or justified all instances of #pragma: no cover; in the case of a bugfix, a new test that breaks as a result of the bug has been added
  • My code contains relevant comments and necessary documentation for future maintainers; the change is reflected in applicable demos/tutorials (with output cleared!) and added/updated docstrings use the NumPy docstring format
  • Any breaking changes, described above, are accompanied by backwards compatibility and deprecation warnings

Copy link

codecov bot commented Nov 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1de4475) 100.00% compared to head (453792d) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #109   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            1         1           
=========================================
  Hits             1         1           

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

@OliviaLynn OliviaLynn changed the title Move in bpz notebooks, update paths and tweak formatting Move in bpz notebooks, update paths; copy over cmnn nb changes Nov 14, 2023
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.

BPZ_lite_with_custom_SEDs.ipynb looks good, but BPZ_lite_demo.ipynb requires one of the files from the tarball that we grab from NERSC. So, I think we need to actually insert a cell in BPZ_lite_demo.ipynb where the user can uncomment and grab those files. I can do that real quick, at which point I'll approve, but I'll wait on merging until you take a look and check that I've done it all correctly.

@sschmidt23 sschmidt23 self-requested a review November 20, 2023 22:32
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.

Ok, I changed some text and added a commented rail get-data command, so now I think it looks good, I'll approve, if my changes look good go ahead and merge.

@OliviaLynn
Copy link
Member Author

Ok, I changed some text and added a commented rail get-data command, so now I think it looks good, I'll approve, if my changes look good go ahead and merge.

Nice catch! That looks great, thank you

@OliviaLynn OliviaLynn merged commit 6194876 into main Nov 21, 2023
6 checks passed
@OliviaLynn OliviaLynn deleted the issue/48/gather branch November 21, 2023 16:35
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.

None yet

2 participants