Skip to content

Tickets/sp 3087a - #196

Open
christinawilliams wants to merge 11 commits into
mainfrom
tickets/SP-3087a
Open

Tickets/sp 3087a#196
christinawilliams wants to merge 11 commits into
mainfrom
tickets/SP-3087a

Conversation

@christinawilliams

Copy link
Copy Markdown
Contributor

adding 303 galaxies series, notebook 303.2 on galaxy shapes

@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

@galaxyumi galaxyumi self-assigned this Aug 14, 2026
@@ -0,0 +1,948 @@
{

@galaxyumi galaxyumi 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.

objectTable, deepCoadd -> Object,deep_coadd


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, thank you!

@@ -0,0 +1,948 @@
{

@galaxyumi galaxyumi 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.

  • object -> Object (this comment applies wherever relevant)
  • "cosmological" -> "cosmological analyses" or "cosmological studies"

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 both. thanks!

@@ -0,0 +1,948 @@
{

@galaxyumi galaxyumi 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.

Could you rearrange these imports? I believe our tutorials generally import standard and third-party libraries first, followed by LSST modules. This change might require updating the text above as well.


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.

updated!

@@ -0,0 +1,948 @@
{

@galaxyumi galaxyumi 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.

I believe that a new standard text for this cell (from Melissa) is:

Instantiate RSPDiscovery with the DP2 release, create an instance of the TAP service, and assert that it exists.


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.

thanks for catching that, text has now been changed

@@ -0,0 +1,948 @@
{

@galaxyumi galaxyumi 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.

Edit suggestion:

"A deep DP2 dataset is in the Extended Chandra Deep Field South (ECDFS)." -> "One of the deep drilling fields in the DP2 is the Extended Chandra Deep Field South (ECDFS)."

Does this galaxy have a widely-used name? If so, please add it. If not, never mind.


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.

Updated to the suggested text. Its not any particular galaxy with a name, it was just a large edge on galaxy i found and used in the DP1 data

@@ -0,0 +1,948 @@
{

@galaxyumi galaxyumi 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.

The output figure looks like collapsed lines at first, so I had to add aspect='auto' inside the plt.imshow statement. But then the figure still looks like black background with white lines. Please double check.


Reply via ReviewNB

@christinawilliams christinawilliams Aug 17, 2026

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.

The issue was that our SIA queries returned overlapping deep_coadds in a different order, so my hard coded indexing to make sure to use the image with full coverage of the galaxy didnt' work. I updated the make_image_cutout function so that it will always use the deep coadd with the fullest coverage for any user regardless of order. Thanks for helping me identify that issue!

@@ -0,0 +1,948 @@
{

@galaxyumi galaxyumi 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.

Please put "Figure 1:" in bold. This comment applies to other figure captions.


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.

updated!

@@ -0,0 +1,948 @@
{

@galaxyumi galaxyumi 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.

It might be useful to add a 2nd panel to show the residual (reconstructed model - observation). This comment applies to Figures 3 and 4.


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.

Yumi and I experimented with this idea. The reconstructed model that includes the PSF convolution seemed like the optimal choice for presenting this, but we think that since the model does not have noise, the percent residual was quite high outside the galaxy and looked a bit weird. We decided to exclude since dealing with noise might be outside scope of the notebook. I saved the code for later in case we write a new notebook to focus on that.

@@ -0,0 +1,948 @@
{

@galaxyumi galaxyumi 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.

This section title doesn't feel very intuitive and seems to interrupt the flow. Would it be better to swap it with Section 6, or perhaps move it under Section 4 as a subsection?


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.

Would "fixed bulge and disk shapes" be better? I intentionally made these their own section at the end because they are new (so were not in the original DP1 notebook) but they are also expected to be removed for DR1 (so then I can just cut the whole section). Would updating just the title adequately address this suggestion?

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.

Yumi confirms this is good (and I moved section 6 to section 5.2 since it now falls into that category)

@@ -0,0 +1,948 @@
{

@galaxyumi galaxyumi 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.

deep_coadd -> deep_coadd


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

Copy link
Copy Markdown
Contributor Author

Thanks Yumi! I addressed all the comments, please let me know if all is good

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