Skip to content

tickets/SP-3204-review-for-release - #185

Open
sgreenstreet wants to merge 6 commits into
mainfrom
tickets/SP-3204
Open

tickets/SP-3204-review-for-release#185
sgreenstreet wants to merge 6 commits into
mainfrom
tickets/SP-3204

Conversation

@sgreenstreet

@sgreenstreet sgreenstreet commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Add 201 series notebooks for current_identifications and numbered_identifications tables.

@review-notebook-app

review-notebook-app Bot commented Jul 27, 2026

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@sgreenstreet
sgreenstreet requested a review from jeffcarlin July 27, 2026 23:54
jeffcarlin
jeffcarlin reviewed Jul 29, 2026
View reviewed changes
Comment thread DP2/200_Data_products/201_Catalogs/201_15_Current_identifications_table.ipynb
@@ -0,0 +1,758 @@
{

@jeffcarlin jeffcarlin Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Be sure to update the link to point to DP2 docs (it's currently dp1).

Also, pipelines version should be r30.0.10.


Reply via ReviewNB

@sgreenstreet sgreenstreet Jul 30, 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.

Thanks for the catch.

@sgreenstreet sgreenstreet Jul 30, 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.

Also updating 201.XX to 201.15.

Comment thread DP2/200_Data_products/201_Catalogs/201_15_Current_identifications_table.ipynb
@@ -0,0 +1,758 @@
{

@jeffcarlin jeffcarlin Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Don't forget to remove the "NEED TO UPDATE" note!


Reply via ReviewNB

@sgreenstreet sgreenstreet Jul 30, 2026 •
edited
Loading

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.

Oops, thought I got all of these, but clearly missed some. Thanks!

Comment thread DP2/200_Data_products/201_Catalogs/201_15_Current_identifications_table.ipynb
@@ -0,0 +1,758 @@
{

@jeffcarlin jeffcarlin Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Update to reflect actual imports.


Reply via ReviewNB

@sgreenstreet sgreenstreet Jul 30, 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.

Done

Comment thread DP2/200_Data_products/201_Catalogs/201_15_Current_identifications_table.ipynb
@@ -0,0 +1,758 @@
{

@jeffcarlin jeffcarlin Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Don't forget to remove "NEED TO UPDATE..." note!


Reply via ReviewNB

@sgreenstreet sgreenstreet Jul 30, 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.

Thanks!

Comment thread DP2/200_Data_products/201_Catalogs/201_15_Current_identifications_table.ipynb
@@ -0,0 +1,758 @@
{

@jeffcarlin jeffcarlin Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there a need to retrieve the full table? You could use "SELECT TOP 10..." or something to demonstrate the query, and it would be much faster.


Reply via ReviewNB

@sgreenstreet sgreenstreet Jul 30, 2026 •
edited
Loading

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.

Comment thread DP2/200_Data_products/201_Catalogs/201_16_Numbered_identifications_table.ipynb
@@ -0,0 +1,648 @@
{

@jeffcarlin jeffcarlin Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I only see 9 columns?


Reply via ReviewNB

@sgreenstreet sgreenstreet Jul 30, 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.

Yes, thanks.

Comment thread DP2/200_Data_products/201_Catalogs/201_16_Numbered_identifications_table.ipynb
@@ -0,0 +1,648 @@
{

@jeffcarlin jeffcarlin Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove the "NEED TO UPDATE" reminder.


Reply via ReviewNB

@sgreenstreet sgreenstreet Jul 30, 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.

Thanks.

Comment thread DP2/200_Data_products/201_Catalogs/201_16_Numbered_identifications_table.ipynb
@@ -0,0 +1,648 @@
{

@jeffcarlin jeffcarlin Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Probably no need to get the entire table? Maybe use "SELECT TOP N...".

Also, "ORDER BY" probably makes this query much slower. I think sorting after returning results would be better (though maybe check our TAP tutorials for advice on this).


Reply via ReviewNB

@sgreenstreet sgreenstreet Jul 30, 2026 •
edited
Loading

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.

Same issue as for the current_identifications table with joins and the workaround provided here.

I tested the query both with and without the "ORDER BY" inclusion and the query time was the same, so I'll leave it as an example of how to order retrieved table info. I also don't know if ordering the two table queries that are merged later for the join workaround makes the dataframe merge faster?

Comment thread DP2/200_Data_products/201_Catalogs/201_16_Numbered_identifications_table.ipynb
@@ -0,0 +1,648 @@
{

@jeffcarlin jeffcarlin Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As noted before, can use results.table.to_pandas(). 


Reply via ReviewNB

@sgreenstreet sgreenstreet Jul 30, 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.

Done.

Comment thread DP2/200_Data_products/201_Catalogs/201_16_Numbered_identifications_table.ipynb
@@ -0,0 +1,648 @@
{

@jeffcarlin jeffcarlin Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

See my comments on NB 201_15 about (a) returning the entire table from your query (is it needed?), (b) using pandas to join (could do it in the query), and (c) counting (can do via TAP).

I would rewrite this section with those changes in mind.


Reply via ReviewNB

@sgreenstreet sgreenstreet Jul 30, 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 workaround was included for the join issues for some SS tables. I think that means this should stay as is for now, and once a fix can be implemented for table joins, the tutorials can be updated, but if this should still be changed, let me know.

jeffcarlin
jeffcarlin requested changes Jul 29, 2026
View reviewed changes

@jeffcarlin jeffcarlin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Left comments in ReviewNB. The main thing is that I think we should avoid queries that return full tables. Demonstrating to users how to query tables doesn't need the whole thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeffcarlin jeffcarlin jeffcarlin requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sgreenstreet @jeffcarlin