astro-datalab / notebooks-latest

Default set of Data Lab notebooks, by DL team and contributed by users
BSD 3-Clause "New" or "Revised" License
60 stars 49 forks source link

DESI EDR Tutorial Notebook with Python 3 Kernel #190

Closed Ragadeepika-Pucha closed 1 year ago

Ragadeepika-Pucha commented 1 year ago

@rnikutta @stephjuneau @jacquesalice @adambolton - This PR is for the DESI EDR notebook that will work with Python 3 Kernel.

Thanks, Raga

Ragadeepika-Pucha commented 1 year ago

@stephjuneau @jacquesalice @rnikutta @adambolton - updated both Py3 notebook with the above suggestions. Fixed some small things in the DESI kernel notebook.

stephjuneau commented 1 year ago

Thanks @Ragadeepika-Pucha! In my opinion, this is now ready to merge (by @jacquesalice or @rnikutta ?)

rnikutta commented 1 year ago

Taking a look right now...

Ragadeepika-Pucha commented 1 year ago

@stephjuneau @rnikutta @jacquesalice @adambolton

@rnikutta - Thank you for the comments. I have implemented most of them - there are a few things that I did not change, like the query comments and the plotting, to be consistent with the other notebook.

Thank you!

jacquesalice commented 1 year ago

Thanks @Ragadeepika-Pucha ! @rnikutta : can you update your review to Approved so we can merge this pull request? It's currently blocked

rnikutta commented 1 year ago

Great, many thanks @Ragadeepika-Pucha ! Just two small things:

rnikutta commented 1 year ago

Hi @Ragadeepika-Pucha , as we discussed yesterday with @stephjuneau , I've added the authClient import myself, and will merge your PR in a second. Thank you for the NB! In a future PR, we can modify the plotting code to define a small function (as suggested in the conversation above), but then make that change in both the DESI and Py3 kernel versions of the NB. It's definitely not urgent.

Ragadeepika-Pucha commented 1 year ago

@rnikutta Thank you for merging the PR! I will add the plotting function to both the notebooks as soon as I can.