Skip to content

SP-2072: add new 303-series tutorial on color-color selections#155

Merged
christinawilliams merged 27 commits into
mainfrom
tickets/SP-2072
Jun 13, 2026
Merged

SP-2072: add new 303-series tutorial on color-color selections#155
christinawilliams merged 27 commits into
mainfrom
tickets/SP-2072

Conversation

@christinawilliams

Copy link
Copy Markdown
Contributor

Adding new DP1 galaxies series 303 notebook on color color selections

@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

@@ -0,0 +1,985 @@
{

@garrethmartin garrethmartin Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

consider adding pyvo and astroquery to the packages section


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added, thanks!

@@ -0,0 +1,985 @@
{

@garrethmartin garrethmartin Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

# 1. Introduction -> ## 1. Introduction


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Whoops thanks for catching that. I also updated the other sections to be consistently hash-tagged.

@@ -0,0 +1,985 @@
{

@garrethmartin garrethmartin Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Check all headings are the correct level e.g.: ## 1., ### 1.1. #### 1.1.1


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,985 @@
{

@garrethmartin garrethmartin Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"!.2." is inconsistent with the rest of the section numbering, which don't have trailing period (i.e. "1.2")


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Whoops I always get confused about this. It looks like the policy is to include the trailing period so I have updated all the other sections that are missing one.

@@ -0,0 +1,985 @@
{

@garrethmartin garrethmartin Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Line #14.        f"CIRCLE('ICRS', {target_ra}, {target_dec}, 0.5)) = 1"

the markdown says 0.2 degrees, but the query of for 0.5 degrees


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I fixed. Search radius 0.5 was needed to maximize the matches.

@@ -0,0 +1,985 @@
{

@garrethmartin garrethmartin Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should read: like Figure 1


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed (and fixed caption for fig 3 as welll)

@@ -0,0 +1,985 @@
{

@garrethmartin garrethmartin Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Galaxies my reside -> Galaxies may reside


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -0,0 +1,985 @@
{

@garrethmartin garrethmartin Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Line #22.            query = f"""

shadows the previous query variable in Section 2.. consider changing


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated query to "cutout_query" thanks for the suggestion

@@ -0,0 +1,985 @@
{

@garrethmartin garrethmartin Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Line #30.            coadds = tap_service.search(query).to_table()

Tutorial guidelines recommend that all TAP queries to be asynchronous


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have updated to asynchronous query.

@@ -0,0 +1,985 @@
{

@garrethmartin garrethmartin Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Line #2.    failed_spec_z = matched_v_all['zsp'][failed_lbg_indices]

I think the filtering contains a bug, np.where returns indices into the already filtered array. so i think it should instead just be:

failed_spec_z = selected_spec_z[selected_spec_z < 3.2]


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, my mistake, thanks for catching.

@garrethmartin garrethmartin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved. See comments for some minor requested changes.

@christinawilliams christinawilliams merged commit 86de243 into main Jun 13, 2026
2 checks passed
@MelissaGraham MelissaGraham changed the title Tickets/sp 2072 SP-2072: add new 303-series tutorial on color-color selections Jun 13, 2026
@christinawilliams christinawilliams deleted the tickets/SP-2072 branch June 13, 2026 02:02
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