tickets/SP-3204-review-for-release - #185
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
| @@ -0,0 +1,758 @@ | |||
| { | |||
There was a problem hiding this comment.
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
| @@ -0,0 +1,758 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,758 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,758 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,758 @@ | |||
| { | |||
There was a problem hiding this comment.
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
| @@ -0,0 +1,648 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,648 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,648 @@ | |||
| { | |||
There was a problem hiding this comment.
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
| @@ -0,0 +1,648 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,648 @@ | |||
| { | |||
There was a problem hiding this comment.
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
jeffcarlin
left a comment
jeffcarlin
left a comment
There was a problem hiding this comment.
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.
2 participants
Add 201 series notebooks for
current_identificationsandnumbered_identificationstables.