SP-2645: added 301 notebook on image display for faint features#158
SP-2645: added 301 notebook on image display for faint features#158garrethmartin wants to merge 10 commits into
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
| @@ -0,0 +1,543 @@ | |||
| { | |||
There was a problem hiding this comment.
Question CST probably has not encountered before: In RTN-045 do we adopt a specific english spelling (British vs American)? Visualise vs visualize, and characterise, etc, but there's many other words like catalog (perhaps we should ask Melissa what to do here). If there's no policy about this then please disregard this comment!
Update from Melissa: we dont' really care about this, except to appease the spellchecker. So perhaps just look through all markdowns prior to merging to check for any other flagged spelling. I will flag here where I noticed the spellchecker flagged in the notebook aspect:
visualise -> visualize, characterise -> characterize
Also regarding lsb-202 and lsb-301, should these be capitalized since its an abbreviation? Maybe its better to list the actual notebook number instead?
Reply via ReviewNB
There was a problem hiding this comment.
Thank you Christina.
Spellings changed to American English for consistency.
Now using correct notebook number to refer to other notebooks in the series.
| @@ -0,0 +1,543 @@ | |||
| { | |||
There was a problem hiding this comment.
Perhaps move the text starting with "two functions are used..." to the next markdown right before function definition
artefact -> artifact
Reply via ReviewNB
There was a problem hiding this comment.
Now split between two code cells w/ markdown before each. Fuller explanation of sigma^ (relevant for the later notebooks)
| @@ -0,0 +1,543 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,543 @@ | |||
| { | |||
There was a problem hiding this comment.
Functions should have the inputs and outputs defined in the commented description e.g.:
def gauss(x, a, x0, sigma):
"""Evaluate a 1D Gaussian function.
Parameters
----------
x : array_like
Input values where the Gaussian is evaluated.
a : float
Amplitude of the Gaussian peak.
x0 : float
Mean (center) of the Gaussian.
sigma : float
Standard deviation (width) of the Gaussian.
Returns
-------
y : array_like
Values of the Gaussian function evaluated at x.
"""
return a * np.exp(-(x - x0)**2 / (2 * sigma**2))
Reply via ReviewNB
There was a problem hiding this comment.
all functions now have complete docstrings
| @@ -0,0 +1,543 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,462 @@ | |||
| { | |||
There was a problem hiding this comment.
Suggest splitting this cell into individual functions or group of related functions (separate from parameter definitions) so that they each get a descriptive markdown to help understand what they do
Also inputs and outputs should be defined in the functions (see example in review in 312.1)
Reply via ReviewNB
There was a problem hiding this comment.
now split up with docstrings added
| @@ -0,0 +1,462 @@ | |||
| { | |||
There was a problem hiding this comment.
Suggest splitting this code block and inserting some narrative markdown at each stage to help the reader
Reply via ReviewNB
There was a problem hiding this comment.
have split and slightly simplified
| @@ -0,0 +1,462 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,462 @@ | |||
| { | |||
There was a problem hiding this comment.
Are there any tips to get a good science measurement for a first-time user? For example avoid background objects or exclude flux from the host or something?
Reply via ReviewNB
There was a problem hiding this comment.
have added some guidance to the markdown cell
| @@ -0,0 +1,462 @@ | |||
| { | |||
There was a problem hiding this comment.
Sections 4-5 are quite fun! Nice
How does one accurately calculate the error on photometry for a weird irregular shape? Is it just the average surface brightness and then scaled by number of pixels in painted region or something?
Reply via ReviewNB
There was a problem hiding this comment.
i've added a basic error calculation to the code, along with a note about how it could be done more robustly
|
Nice! I left lots of comments and suggestions in the 3 notebooks. Please let me know if you have any questions about any of it! Nice job. |
christinawilliams
left a comment
christinawilliams
left a comment
There was a problem hiding this comment.
Left some comments and feedback in the 3 notebooks, please let me know if there are any questions! Nice job!
…are too few valid pixels
|
thank you Christina, i've addressed all your comments. |
| @@ -0,0 +1,608 @@ | |||
| { | |||
There was a problem hiding this comment.
Sorry if I missed this before: I think its customary to put the imports and settings definitions in different cells (and probably the latter are considered "defining parameters" below?
Reply via ReviewNB
| @@ -0,0 +1,608 @@ | |||
| { | |||
There was a problem hiding this comment.
Is it possible something didn't render correctly for lo_sigma and hi_sigma? I didn't understand these two parameters
Reply via ReviewNB
| @@ -0,0 +1,608 @@ | |||
| { | |||
There was a problem hiding this comment.
RTN-045 doens't specifically say that each pre-defined functions should be in their own cell, but I think most notebooks I've used do that? Perhaps check with Melissa, suggest putting in different cells but this is perhaps not necessary.
Reply via ReviewNB
| @@ -0,0 +1,608 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,1243 @@ | |||
| { | |||
There was a problem hiding this comment.
Suggest moving the parameter definitions to section 1.2 with title define parameters
Reply via ReviewNB
| @@ -0,0 +1,1243 @@ | |||
| { | |||
There was a problem hiding this comment.
Suggest splitting these in 3 cells (instantiate butler, identify patch/tract, retreive image and extract wcs params)
Reply via ReviewNB
| @@ -0,0 +1,692 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,692 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,692 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,692 @@ | |||
| { | |||
There was a problem hiding this comment.
|
Nice! I made a few last minute minor suggestions but it looks good to go! Congrats! |
christinawilliams
left a comment
christinawilliams
left a comment
There was a problem hiding this comment.
Looks good! I left a few last minute suggestions but after that I think you're good to go! I left it approved so you can do it whenever ready.
3 participants
SP-2645: new tutorial on image display