Skip to content

SP-3313: review and improve the 301 series NBs - #195

Merged
jeffcarlin merged 4 commits into
mainfrom
tickets/SP-3313
Aug 17, 2026
Merged

SP-3313: review and improve the 301 series NBs#195
jeffcarlin merged 4 commits into
mainfrom
tickets/SP-3313

Conversation

@jeffcarlin

Copy link
Copy Markdown
Collaborator

Made minor updates to use SkyCoord for calculating separations, and added some small science demos.

@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@GloriaFA GloriaFA self-assigned this Aug 14, 2026
@@ -22,7 +22,7 @@
"Data Release: [Data Preview 2](http://dp2.lsst.io) \\\n",

@GloriaFA GloriaFA Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the pipelines version be updated?


Reply via ReviewNB

@@ -22,7 +22,7 @@
"Data Release: [Data Preview 2](http://dp2.lsst.io) \\\n",

@GloriaFA GloriaFA Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Line #19.        "Rubin_SV_320_-15": [320.2, -15.1, 4.51],

Would it make more sense to leave it as 4.5 to match the other fields?


Reply via ReviewNB

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately the number of visits I get with 4.5 is not the same as what's reported in the documentation (because when Melissa calculated those originally she used a different calculation). To get the numbers to match up I had to extend it to 4.51.

@@ -22,7 +22,7 @@
"Data Release: [Data Preview 2](http://dp2.lsst.io) \\\n",

@GloriaFA GloriaFA Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Line #6.    tx = np.where(sc_region.separation(sc_patches).value <= rad)[0]

Is this supposed to be <= radius instead? The plot produced is more zoomed in and I wasn't sure if that was an intentional choice or a copy paste error.

tx = np.where(sc_region.separation(sc_patches).value <= radius)[0]

Though I noticed that doing this shows more patches around the corners than in the original notebook.


Reply via ReviewNB

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes - good catch!

@@ -22,7 +22,7 @@
"Data Release: [Data Preview 2](http://dp2.lsst.io) \\\n",

@GloriaFA GloriaFA Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Line #7.        # offsets = np.sqrt((joined_table['ra']-ra)**2 + (joined_table['dec']-dec)**2)

Forgot to remove commented out old code


Reply via ReviewNB

@@ -22,7 +22,7 @@
"Data Release: [Data Preview 2](http://dp2.lsst.io) \\\n",

@GloriaFA GloriaFA Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe "has few stellar features in the color-color diagrams"


Reply via ReviewNB

@@ -22,7 +22,7 @@
"Data Release: [Data Preview 2](http://dp2.lsst.io) \\\n",

@GloriaFA GloriaFA Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Line #8.    #     delta_ra = (coords[0] - patches_table['s_ra'])*(np.cos(np.deg2rad(coords[1])))

Lines 8-11, forgot to remove commented out old code


Reply via ReviewNB

@GloriaFA GloriaFA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed all 3 updated notebooks and left small comments.

@jeffcarlin
jeffcarlin merged commit 73fc32f into main Aug 17, 2026
1 of 2 checks passed
@jeffcarlin
jeffcarlin deleted the tickets/SP-3313 branch August 17, 2026 18:47
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.

2 participants