Skip to content

DM-56333: DP2 NB: 201_17 tutorial on the visit_summary table - #222

Merged
jrob93 merged 6 commits into
mainfrom
tickets/DM-56333
Oct 13, 2026
Merged

jrob93 merged 6 commits into
mainfrom
tickets/DM-56333

Conversation

@jrob93

@jrob93 jrob93 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

First draft of the visit_summary butler only table tutorial. Based primarily on the VisitDetector tutorial due to overlapping columns.

Also includes snippets from 204.3. Astrometric Calibration and 205.2. PSF for visits because these tutorials use visit_summary as a workaround for visit_images not being available in EDP2.

Fro ticket https://rubinobs.atlassian.net/browse/DM-56333

@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

@garrethmartin garrethmartin self-assigned this Oct 9, 2026
@@ -0,0 +1,1046 @@
{

@garrethmartin garrethmartin Oct 9, 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.

make sure flake8 runs clean. there are a few failures.

$ flake8-nb DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb --max-line-length=99 --ignore=E133,E226,E228,E266,N802,N803,N806,N812,N813,N815,N816,W503

DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb#In[ ]:3:1: E124 closing bracket does not match visual indentation

DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb#In[ ]:4:1: E124 closing bracket does not match visual indentation

DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb#In[ ]:3:27: E128 continuation line under-indented for visual indent

DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb#In[ ]:3:100: E501 line too long (104 > 99 characters)

DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb#In[ ]:4:27: E124 closing bracket does not match visual indentation

DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb#In[ ]:2:6: E231 missing whitespace after ','

DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb#In[ ]:1:53: E231 missing whitespace after ','

DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb#In[ ]:5:62: E231 missing whitespace after ','

DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb#In[ ]:23:14: E231 missing whitespace after ','

DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb#In[ ]:23:16: E231 missing whitespace after ','

DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb#In[ ]:23:18: E231 missing whitespace after ','

DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb#In[ ]:23:29: E231 missing whitespace after ','

DP2/200_Data_products/201_Catalogs/201_17_Visit_summary_table.ipynb#In[ ]:23:56: E231 missing whitespace after ','


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.

Ah good catch, I completely forgot to run flake8!

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'm left with some W291 trailing whitespace flags that I'm not 100% sure how to resolve.

@garrethmartin garrethmartin Oct 13, 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.

if you run flake8 with --select=W291 --show-source it should show you exactly where the issue is

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 the tip! Done.

@@ -0,0 +1,1046 @@
{

@garrethmartin garrethmartin Oct 9, 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 add some of the other more substantially used packages e.g. lsst.obs.lsst.LsstCam, lsst.images plus numpy etc


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 lsst.images, lsst.obs.lsst

@@ -0,0 +1,1046 @@
{

@garrethmartin garrethmartin Oct 9, 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.

per RTN-045, no need to mention tutorials in the same series


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.

Removed 201.11. I didn't realise this before so to be clear I think series is 201, so the other 200 level tutorials should be kept?

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.

yes. i think so

@@ -0,0 +1,1046 @@
{

@garrethmartin garrethmartin Oct 9, 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.

good to include assert statements for things like this e.g

assert butler is not None


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,1046 @@
{

@garrethmartin garrethmartin Oct 9, 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.

is it worth briefly explaining what an ExposureCatalog/ExposureRecord actually is compared to e.g. a SourceCatalog?


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 a bit more description and a link to https://pipelines.lsst.io/modules/lsst.afw.table/index.html

@@ -0,0 +1,1046 @@
{

@garrethmartin garrethmartin Oct 9, 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.

2.1.4. -> 2.2.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.

Done.

@@ -0,0 +1,1046 @@
{

@garrethmartin garrethmartin Oct 9, 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.

"visits within 2 degrees" is this correct for OVERLAPS POINT? i think it only takes a position.


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 spot!

@@ -0,0 +1,1046 @@
{

@garrethmartin garrethmartin Oct 9, 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 #3.    search_radius = 2

not actually used


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 spot!

@@ -0,0 +1,1046 @@
{

@garrethmartin garrethmartin Oct 9, 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 #2.    for i,x in enumerate(visit_summary_tab.colnames):

avoid single character variable names


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,1046 @@
{

@garrethmartin garrethmartin Oct 9, 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.

x and y axes and the colorbar should have units

and change single character variable names


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.

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

please merge after addressing minor comments

@jrob93
jrob93 merged commit 8a94aa6 into main Oct 13, 2026
2 checks passed
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