Closed weaverba137 closed 1 year ago
@jacquesalice @rnikutta @stephjuneau, today is a very busy day, but I hope I can test this later today or on Tuesday.
One question is: does this notebook overlap in functionality with other DESI EDR notebooks that are also in the pipeline?
Hi Ben, possibly a little bit of overlap with the Intro to DESI EDR notebook here (https://github.com/astro-datalab/notebooks-latest/pull/175) which I'm reviewing. However, I will request a change to use only the zpix
table for the main query for that one. This means that there will no longer be an example joint query so it will be valuable to have several different queries like the ones demonstrated in the database notebook. (That said, tomorrow will be fine for the database notebook if today is tight!)
Thank you @stephjuneau!
Thank you for the comments. This is indeed still a work in progress, so I will be sure to incorporate the suggested changes.
@jacquesalice @stephjuneau this is now ready for review.
Hi @weaverba137 Hoping that it helps speeding up the merge process, I've just reviewed your NB. It runs a flows very well. Just a few small comments:
Remove the following imports, they are not used:
The NB mentions slice notation in Sections "Redshift and classification" and "Another simple join", but no slice notation is used. Remove references to slice notation?
For the cell starting with this comment # Avoid warnings about invalid values in np.log10().
, disable scrolling outputs (right-click on the output cell, select "Disable scrolling outputs"). Otherwise the plot is partially hidden.
In "Survey progress", could add a small figure:
plt.bar(pd.to_datetime(response['night'],format="%Y%m%d"),response['n_exp'],width=1)
plt.xlabel('date')
plt.ylabel('n_exp')
(needs an import pandas as pd
up front)
%%time
# This will run for ~1 minute
# foo = axes.legend(loc=1)
For the location of the NB in the tree, I'm swinging between here, and 03_ScienceExamples/DESI/, but think I prefer here more, as it's a technical NB.
OK, I've addressed everything, please take a look. However, I kept the from desitarget.targetmask import (desi_mask, mws_mask, bgs_mask)
, and now I'm actually using desi_mask
.
I've also added an HTML version.
I can make further changes until about 12:00 MST, but earlier would be better.
Thanks @weaverba137
desi_mask
. But mws_mask
and bgs_mask
are not being used, right? So let's not import them.ac.whoAmI()
line? This will prevent the username who last ran the NB to show. If...__version__
string to today, and we can merge.
Many thanks!OK, I'll make those changes momentarily.
OK, should be good to go.
@jacquesalice Can you please sign off on your review of @weaverba137 's NB? I currently can't merge (without overriding) unless you sign off on it ;-) Thanks.
Hi @rnikutta @weaverba137 @jacquesalice I think it's already been agreed to leave this notebook in the HowTos/ folder. I wanted to mention that I suggesting linking it among a compilation of notebook links for a short Mirror article on DESI EDR. I'm not sure whether the compilation of links will make it to the final publication, but I'm giving it a try! Also, I propose adding a README file to the 03_ScienceExamples/DESI/ folder to point to that notebook and to the HowTo on SPARCL. I will work on this today and send it for approval.
Sounds good @stephjuneau . I'll merge after your commit.
Hi @rnikutta this branch doesn't have the DESI/ folder with Raga's notebooks. Should I go ahead and add those as well then? That's the folder where I'm planning on adding a README.
Edit: I'm also happy to do a separate branch after this one is merged.
Better start a new branch, and a new PR @stephjuneau . I'll merge Ben's branch now, so that it shows up on master. (sorry @jacquesalice , going to override!)
@stephjuneau Merged!
This PR adds a notebook that tries to replicate the functionality of DESI's official database tutorial as closely as possible.
The official notebook has not yet been finalized, so this PR is also a Work In Progress.