search

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 48 forks source link

Update NB template: disclaimer and acks, available datasets section, … #225

Closed rnikutta closed 6 months ago

rnikutta commented 6 months ago

…commented out login cell.

rnikutta commented 6 months ago

@stephjuneau @jacquesalice @galaxyumi I've updated the DL NB template:

I also edited the cell that lists all available datasets, and commented out the Login cell.

Please kindly review the template NB. The Disclaimer and Acks cell will be used to update all other NBs in the next step.

rnikutta commented 6 months ago

It's a good point.. The reason I commented it out is because in most cases (I think) a user running a NB will be logged in, including when we test the NBs ourselves (which then always evaluates to my username...). To avoid having to explicitly use ac.logout(), then remove that code again, and re-run the NB, I thought it was easier to comment out the cell. The text just before the auth cell actually says explicitly to un-comment the cell, should a login be needed.

What do you think?

jacquesalice commented 6 months ago

It's a good point.. The reason I commented it out is because in most cases (I think) a user running a NB will be logged in, including when we test the NBs ourselves (which then always evaluates to my username...). To avoid having to explicitly use ac.logout(), then remove that code again, and re-run the NB, I thought it was easier to comment out the cell. The text just before the auth cell actually says explicitly to un-comment the cell, should a login be needed.

What do you think?

If we comment it out in this notebook, should we comment it out in all notebooks?

rnikutta commented 6 months ago

I've been suggesting as much in most of the NBs that I've been reviewing over the past months..

rnikutta commented 6 months ago

(though I don't think we need to start a big campaign to change that everywhere. But if we get to edit a NB for any reason down the line, this can be changed then.)

stephjuneau commented 6 months ago

I understand the argument better for testing although maybe it could show being logged as "demo00". I still think that there is some value in showing that the user is logged in or not but it's a pretty small change if we were to change our mind so I'll go ahead and approve the PR as well. I also agree that we don't need a concerted effort to change in all notebooks.

rnikutta commented 6 months ago

Thank you, will merge!