Added a file, 03_ScienceExamples/DESI/03_DESI_AGNQSO_VAC_tutorial.ipy… #287
Conversation
…nb, as an adapted tutorial from DESI to work on the Astro Data Lab platform. Changed 03_ScienceExamples/TimeSeriesAnalysisRrLyraeStar/TimeSeriesAnalysisOfRrLyraeStar.ipynb to nbid 0076 because it was sharing nbid 0031 with 03_ScienceExamples/StarGalQSOSeparation/StarGalQsoLSDR9.ipynb. nbid was updated in both nbid.csv and the RRLyrae notebook.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
@rnikutta Stephanie said to tag you since I changed the nbid of the 03_ScienceExamples/TimeSeriesAnalysisRrLyraeStar/TimeSeriesAnalysisOfRrLyraeStar.ipynb file from 0031 to 0076 because it was sharing nbid 0031 with 03_ScienceExamples/StarGalQSOSeparation/StarGalQsoLSDR9_master.ipynb |
|
Thank you @Astro-Morgan , good catch with the duplicate nbid. 0075 and 0076 look good. |
There was a problem hiding this comment.
Hi Andy, great work! I think I managed to get all my most important comments in this review. I might do another round of review with minor suggestions after you make your next commit. Let me know if you have questions on my review or anything you'd like to discuss. Thanks!
There was a problem hiding this comment.
Overall this very nice work, thank you! This notebook will be a useful addition to the Data Lab notebook collection!
I'm including my list of comments and suggestions below, in the order of the notebook (including one thing that I forgot to mention at first)
Top cell
can remove some of the commented out instructions and I'm making some keyword suggestions
__nbid__ = '0075' # ID of this notebook in the DL NB collection
__author__ = 'Andy Morgan <[email protected]>, Stephanie Juneau <[email protected]>'
__version__ = '20250904' # yyyymmdd
__datasets__ = ['DESI DR1']
__keywords__ = ['tutorial', 'galaxies', 'quasars', 'sparcl', 'spectroscopic redshift']
(delete 'HowTo' from keywords as this is a different notebook category for the Data Lab; while Python and Jupyter are correct, I think we tend to use those for beginner notebooks that emphasize how to use Python and explain how Jupyter notebooks work)
Summary
- I think it'd be nice to add a couple of motivation sentences to justify why one might be interested in using this catalog, which can be adapted from the Readme file or original notebook or I can help and make a suggestion
- spell out "ADL's" as "Astro Data Lab's" because there are so many acronyms already
Acknowledgments
Under "If you use the DESI DR1 AGN/QSO VAC and/or this example notebook:", please add a bullet:
- Review and follow the official DESI Data License and Acknowledgments
Resources and references
This section is a place-holder from the template. I think you could remove it since the references are included throughout the notebook in this case. (Otherwise, you could move it at the end and list the link to the DESI Readme and tutorial as well as copy/paste the references there)
Imports
- agnmask.yaml file should be included with the notebook in the same folder (rather than asking the user to add it). Could you please upload it with your branch/PR?
- something I forgot to mention (my bad!) is that we have DESI software pre-installed at the Data Lab but one needs to select the "DESI 25.3" kernel for the notebook rather than the default Python 3 kernel. Could you please:
- change the kernel
- add instructions in the notebook to select the DESI 25.3 kernel
- replace the instruction to pip install this code with just an import line:
# DESI
from desiutil.bitmask import BitMask
Create a Function to Decode the Bitmasks
Move the imports with the rest of the imports in the earlier section
Accessing the AGN/QSO Catalog
"The public location of " <-- replace with "The description of "
Filtering AGN/QSO with Bitmasks
Make small changes to the comments in the first cell to reflect that the yaml file is included alongside the notebook (assuming you will add it)
How to Plot Distributions
I like your idea to obtain more objects to make more interesting plots! I suggest moving the new query into its own cell to make it stand out a bit more as it will then be used for all the future plotting in the notebook.
The Other Diagnostics
Rename this section as "Additional AGN Diagnostic Diagrams"
1) The He II BPT
I know we call it the "He II BPT" in the original notebook, but let's use a more generic name "The He II Diagnostic Diagram" as this diagram is not as well known.
"log([HEII]" <-- the "e" is lower case as He is for Helium and no square brackets because this is a permitted line and square brackets are for forbidden lines, so "log(He II..."
At the bottom of the He II plot: "The plot above looks pretty sparse for our measly 1,000,000 galaxy initial sample" <-- It's interesting that it looks sparse given that 1e6 points would normally not be sparse. So this implies that the He II line is rarely detected and in particular, very rarely detected in non-AGN (very few blue points in the Star-Forming side). I suggest rephrasing such as: "The plot above looks pretty sparse despite starting from 1,000,000 galaxy initial sample, indicating that the He II emission line is rarely detected in DESI spectra, in particular for star-forming galaxies."
4) MEx : Mass Excitation Diagnostic Diagram
Could you extend the mass_range variable to start at smaller values so that the black lines reach the left-hand side of the plot?
Also, for both the MEx and KEx (and this is something that's been bugging me in the original diagram), it'd be great to replace the fixed gridsize value by something that instead gives us more equal bin sizes on the plot. The issue is that the different regions have different ranges so applying a fixed gridsize = ends up creating tiny bins in the intermediate / compact regions of the plot.
…3, agnmask.yaml placed in notebook directory
|
View / edit / reply to this conversation on ReviewNB stephjuneau commented on 2025-10-07T03:55:42Z *I suggest some small changes for the following version for the main paragraph:** "The DESI survey is the largest collection of extragalactic spectra assembled to date. DESI DR1 includes optical spectra for an unprecedented 13.1 million galaxies and 1.6 million quasars, reaching redshifts z ~ 6. Every object in DESI DR1 has Redrock spectral types and redshifts, and is some cases classifications based on the QuasarNet and MgII post-processing pipelines ([Chaussidon et al. 2023](https://ui.adsabs.harvard.edu/abs/2023ApJ...944..107C/abstract)). The AGN/QSO VAC builds on those products by assigning Active Galactic Nuclei and/or quasar classifications to each object using emission-line and photometric diagnostics. This classification make it easy to distinguish AGN from star-forming galaxies (and other classes), enabling both targeted studies of specific populations and direct comparisons between them."
**Changes in brackets for the bullets:** - Note: the Astro Data Lab version of the DESI DR1 AGN/QSO VAC uses lowercase column names as opposed to the DESI hosted [database <-- file] accessed in the tutorial above Also: [by reading two file extensions,] the above tutorial makes two tables, T and T2 with differing columns. Astro Data Lab [has these combined <-- serves all columns in a single table], so we will only use "T" in this tutorial |
|
View / edit / reply to this conversation on ReviewNB stephjuneau commented on 2025-10-07T03:55:43Z - be sure to have the kernel set to 'DESI 25.3'. <-- be sure to select the 'DESI 25.3' kernel |
|
View / edit / reply to this conversation on ReviewNB stephjuneau commented on 2025-10-07T03:55:44Z
|
|
View / edit / reply to this conversation on ReviewNB stephjuneau commented on 2025-10-07T03:55:44Z Line #2. # we can also move these filtered objects to a new table Not sure we want to create a new table as this can use a lot of RAM when working with large tables, but could show how to print the first five entries by having the last line as: |
|
View / edit / reply to this conversation on ReviewNB stephjuneau commented on 2025-10-07T03:55:45Z This is still okay because 1e6 rows is not *that many* but if a user wanted to query even more rows, it's better to select the subset of columns rather than all columns (though in this notebook it'd be a long list of columns given all the diagnostic plots)... probably ok like this for now. |
|
View / edit / reply to this conversation on ReviewNB stephjuneau commented on 2025-10-07T03:55:46Z Line #1. # Now we create our hexbin plot, this time with a line demarking the barrier decided on by Shirazi & Brinchmann "with the demarcation line prescribed by Shirazi & Brinchmann" |
|
View / edit / reply to this conversation on ReviewNB stephjuneau commented on 2025-10-07T03:55:46Z "... in the star-forming region despite using an initial sample of 1,000,000 galaxies. This indicates ..." |
|
View / edit / reply to this conversation on ReviewNB stephjuneau commented on 2025-10-07T03:55:47Z just "Blue Diagram" singular in the title and text. While there are two papers, they correspond to the original and revised version of the same diagnostic diagram. |
|
View / edit / reply to this conversation on ReviewNB stephjuneau commented on 2025-10-07T03:55:47Z Line #32. plt.ylabel('log( \[OIII\]/H$\\beta$)')
\[OIII\] shows up with the extra |
|
View / edit / reply to this conversation on ReviewNB stephjuneau commented on 2025-10-07T03:55:48Z Maybe the norm values need adjusting so the color levels are better balanced (and/or grid size could be adjusted to larger bins) |
|
View / edit / reply to this conversation on ReviewNB stephjuneau commented on 2025-10-07T03:55:48Z "5" --> "Six sets of ..." |
|
@Astro-Morgan (Cc @jacquesalice ), I'm done with my second round of comments on the DESI DR1 -- AGN/QSO VAC notebook. Overall very nice work! P.S. I see there is now a conflict with the nbid.csv file, which will need to be checked as it could be with the duplicate nb 76 (or this one which was supposed to become 75) being used in a different PR since it took me a long time to get back to reviewing this (my bad :-/ ). Hopefully it won't be hard to resolve |
|
View / edit / reply to this conversation on ReviewNB jacquesalice commented on 2025-10-07T18:04:02Z Line #4. __datasets__ = ['DESI DR1'] Change to 'desi_dr1' |
|
View / edit / reply to this conversation on ReviewNB jacquesalice commented on 2025-10-07T18:04:03Z To match our other notebooks, all section titles should be lowercase after the first word. For instance, change "Creating BPT Diagnostic Diagrams" to "Creating BPT diagnostic diagrams". Both here and in the actual sections of the notebook.
Also, remove the "Resources and references" point since it doesn't exist in this notebook |
|
View / edit / reply to this conversation on ReviewNB jacquesalice commented on 2025-10-07T18:04:03Z This cell also needs to include the getpass import. That is, from getpass import getpass |
|
View / edit / reply to this conversation on ReviewNB jacquesalice commented on 2025-10-07T18:04:04Z Since you use latex in other parts of this notebook, change "2^11" to "2$^{11}$" |
|
View / edit / reply to this conversation on ReviewNB jacquesalice commented on 2025-10-07T18:04:05Z Line #2. # Acceptable formats are 'csv', numpy 'array', numpy 'structarray', pandas 'dataframe', astropy 'table', astropy 'votable', and 'ascii' Change to: # Acceptable formats are 'csv', numpy 'structarray', 'pandas' dataframe, astropy 'table', astropy 'votable', and 'ascii'
|
|
View / edit / reply to this conversation on ReviewNB jacquesalice commented on 2025-10-07T18:04:06Z Unless you need it for some reason unbeknownst to me, remove the "\" in the axes names. Like: plt.xlabel('log( EW([OII]$_{3726, 3729}$)/EW(H$\\beta$) )')
plt.ylabel('log( [OIII]$_{5007}$/H$\\beta$ )')
|
There was a problem hiding this comment.
@Astro-Morgan thanks for applying the changes! I marked the ones you completed as "Resolve Conversation" in the reviewnb app. Though there were a few that you missed, if you can go back and fix those:
https://app.reviewnb.com/astro-datalab/notebooks-latest/pull/287/files#comment-diff-3378000970
https://app.reviewnb.com/astro-datalab/notebooks-latest/pull/287/files/#comment-diff-3375099162
https://app.reviewnb.com/astro-datalab/notebooks-latest/pull/287/files/#comment-diff-3375099185
|
@stephjuneau will have to Approve as well before I can merge |
…nb, as an adapted tutorial from DESI to work on the Astro Data Lab platform. Changed 03_ScienceExamples/TimeSeriesAnalysisRrLyraeStar/TimeSeriesAnalysisOfRrLyraeStar.ipynb to nbid 0076 because it was sharing nbid 0031 with 03_ScienceExamples/StarGalQSOSeparation/StarGalQsoLSDR9.ipynb. nbid was updated in both nbid.csv and the RRLyrae notebook.