Photutils tutorial notebook #3: aperture photometry #4
Conversation
This reverts commit 030657f.
laurenmarietta
changed the title
|
Apologies for the weird formatting of this 'review', the commit history for this notebook means I can't just review the diff between this branch and master, so I'm afraid I have to do this review by hand. The line in question will be permalinked and the comments/suggestions etc. will follow it. In answer to your three questions above:
================ Very minor nitpick, but shouldn't this be the flux of a celestial source? If you think yours is right ignore this, but I think this way reads better, with the flux belonging to the source.It might also be good to add that the flux is measured on an image (of a CCD/detector) with measures the amount of flux within a region of an astronomical image of defined shape and size.Very minor change too, I think the flux emitted by a desired source and none of the flux emitted by the surrounding sky might be more correct (with the flux belonging to a source), but I'm not 100% on that so again if you think the original way is right then ignore this.
================ As with the previous review, these boxes don't display correctly for me without empty lines between the <div> blocks and the text.
================ (I'm going to skip the ================ Super minor comment, but I think this should be a previous notebook rather than the previous notebook, as that would then give the flexibility of adding a new notebook at position 2.5 and still have this sentence be fine without editing.
================ (Super generic comment here again about updating all deprecation warnings mentioned in the previous notebook here as well, such as ================ Super minor change but perhaps this (and others below) should be print(variable) instead of just variable? It's the same in a notebook I think but a minor change that would allow a user to scoop up all of the cells and make a .py file without the script falling over, and still allow them to verify they got the same result as the notebook did. Not sure whether that's official policy, but I know I prefer working in python scripts rather than notebooks eventually.
================ Slightly vague comment here, but I'm not quite sure what we're doing at this point. The only context I've been given is that we can create apertures, then we've searched data for sources and plotted them (fine, this part is a replication of a previous notebook and doesn't need much explanation), but perhaps before the Circular Apertures section there could be a mini paragraph along the lines of "Ok, now that we've found our sources, what can we do with them? We want the calculate their fluxes, so we need to get some apertures inside which we can sum the count rates of all of the pixels. We first try circular apertures as simple examples..." or some such; something that semi-reminds the user what they're doing and why it's useful. There basically just isn't much text from you (as the creator/showrunner of the notebook) in quite a few cells of code, and I really liked in notebook 2 how much explanation there was at each step of the way for the user (also stuff like
which is a nice little section of explanation!)
================ Minor comment, but perhaps this line could be something like aren't any nice round nearby Milky Way stars in this image! to make the obvious point more bluntly that stars are round, galaxies are not? Not much of an issue otherwise just thought it'd be good to go a bit overboard with the "these are galaxies in the XDF!" schtick.
================ Similar to above, I think this could perhaps do with a link to notebook 2, with a "see previously for what we did here" to remind the user there's a notebook available to figure out what this thing does and why it's neat. I was briefly thrown as to why the ellipses were all unique until I remembered what detect_sources does myself...
================ Similar to above, print(table) instead of table would be good here.
================ Similar to the ending of the Circular Aperture section, just before this section begins the elliptical aperture section could do with a closing sentence/paragraph, with something like "as you can see these apertures fit much better, combined with the powerful tools available in the detections toolkit and detect_sources much better than circles for galaxies in XDF..."
================ This could perhaps add produce a WCS object from the header of the FITS file containing our image, to make it a bit clearer that the header relates to the picture you're working on.
================ Super minor typo: data array on which, not one which.
================ Also super minor: should SkyAperture be inside tick quotes, being a photutils thingy?
================ Not sure whether this matters much, but I got a warning about the meta attribute being set on the data object but ignored by the function. If the meta flag is used above then I guess this is unavoidable, but otherwise we can likely just remove it to remove a slightly weird not-quite-error message from the notebook.
================ Just a reminder that if you change the units from u.ct to u.electron, this cell will also need updating.
================ To be consistent with below, the "r = X" entries in this line and the one below it could be tick quoted ( r = 3 etc.), as they are in the next section.
================ This cell could again be print(phot_table) instead of just phot_table. It could also perhaps have a little sentence lead in of "now that we've (eventually!) made all of our distinct aperture sums for all of our distinct positions, let's see the results:"
Linked with the above line, this could then also get dressed up a little along the lines of "just printing the results doesn't really tell us much, so let's explore alternative ways to examine these data:" (also these instead of this to be consistent with plural data used throughout the rest of the notebook). ================ These plots (and any below, and any above I didn't notice) either just need a unit change to electrons if the swap is made, or to make use of the fancy metadata units plot code Erik mentioned in notebook 2 to print the units of the datacube. Also not sure if it's particularly necessary but for explicit statement you could add a y label just of 'Number of sources' or something. ================ Same issue as above and in notebook 2 where the formatting is shown here unless the lines are separated by line breaks. It also seems that <h3> doesn't accept ** so those just need removing I think.
================ Not 100% sure this is necessary as I'm not sure what level we're quite pitching these notebooks at, but a very brief half-sentence on what an aperture annulus is and how we can use it to background subtract (get average counts in pixels near but outside of object of interest, assumed to be free of source flux, and subtract that flux from all pixels inside the source aperture) might be good just to give a super quick broad context for the start of a new section. (it seems there kind of is one of these at so I'm not sure whether this comment should be changed now but I'm gonna leave it as it was and just suggest perhaps that this latter section is moved up to the previous part I wanted this context in) ================ Should background value be something like the count rate of the background or the background count rate? value sounds a bit vague, could do with being a little bit more specific I think. Are you worried it gets confused with count rate within each annulus in the first step?
================ Here and cell two down should maybe be print(variable) instead of just variable again.
================ Might be good here to have a brief little sentence about the fact that because these data are already background subtracted we'd hope that the background is tiny and oh look it is, just to reassure users that this function does have use honest and that 0.0007 ct/s is fine as a value to get... I also think this should possibly have units u.electron / u.s / u.pixel (or u.ct) as that makes it clear that this is the count rate in a single pixel.
================ Hopefully this review is ok and makes sense - let me know if you need any clarification of anywhere I've tried to link to or not formatted right, etc. |
|
Thanks so much for this great feedback, @Onoddil! Again, per the new plan for finishing up these notebooks, I am going to close this PR - any future discussion will take place on the spacetelescope/notebooks repo, on the new PR thread: spacetelescope/notebooks#101 I have implemented almost all your thoughts, though, and I very much appreciate your attention to detail. I am choosing not to use some of your suggestions though, including:
|
The third notebook tutorial for
photutils, demonstrating methods for aperture photometry.Some preliminary concerns:
Tagging @eteq and @Onoddil for review.