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

Fix AntaresFilterDevKit notebook outdated documentation #165

Closed sivicencio closed 1 year ago

sivicencio commented 1 year ago

Hello from the ANTARES dev team!

The AntaresFilterDevKit notebook needs some updates as follows (mostly documentation-related):

I was not sure of how to update the HTML file associated to the notebook. What I did was to click (inside Data Lab) on the notebook in the file browser at the left side . Then click on File -> Export Notebook As -> HTML. Let me know if it's ok or if I need to update it differently.

Thanks!

sivicencio commented 1 year ago

Hi @rnikutta and @jacquesalice,

Thank you for your thorough review! The requested changes are definitely going in the direction of having a better notebook for the community.

We discussed some of the changes with the team and I'd like to mention a couple of things to you:

  1. The warning messages that you see in different places are due to pandas code that we're using in the DevKit and also to the observability tool we're currently using in ANTARES. For the former, we could fix it but we'd need to prioritize it first. For the latter, we'll be working on changes to the observability tool in the near future that should remove the jaeger warning. For both cases, we don't think it's something we can have ready soon. Please note that most of these warnings are present in the current notebook as well (they were not introduced with this PR). So if it's ok with you, we could leave them temporarily until the mentioned tasks are done
  2. The https change in the URL of ANTARES will be solved in the next production release (which should be done in the next week or couple of weeks)
  3. We agree to reduce the n parameter passed to run_many to n = 10

I'll be working on the other requested changes, but since we need a new ANTARES production release to fix all of them, I was thinking about re-running the notebook after that release, commit the changes and then re-request your review.

Let me know if this works for you 🙌🏼

jacquesalice commented 1 year ago

@sivicencio that sounds good!

sivicencio commented 1 year ago

Hi @rnikutta and @jacquesalice,

After some time I added the requested changes and this PR is ready for review again.

Thank you.

sivicencio commented 1 year ago

@jacquesalice Glad that you could ran that cell! The error was probably related to some test data we got ingested by error in ANTARES, and which was likely incomplete. That was solved yesterday so it makes sense with the timing. Let me know anything else you encounter!

jacquesalice commented 1 year ago

I agree with Robert's edits and have no other suggestions

sivicencio commented 1 year ago

Great, thanks to both of you for your review, @rnikutta and @jacquesalice.

I pushed one more commit with the requested changes. However, I was not able to find the two empty cells in section 4.3. Let me know if I'm missing anything.

rnikutta commented 1 year ago

Thank you @sivicencio ! Merging.