Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add spectral-cube reprojection tutorial #504

Merged
merged 11 commits into from
Apr 1, 2022

Conversation

keflavich
Copy link
Contributor

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@adrn
Copy link
Member

adrn commented Nov 23, 2021

Sorry for the build failures - I just fixed those in #505 - do you mind rebasing on main?

@jonathansick
Copy link
Contributor

The specific technical build issue here is with the kernel that the notebook is saved with. I've created an issue because we should definitely handle this gracefully on our end. astropy/nbcollection#16

In the meantime, It looks like the notebooks we are running use a generic python3 kernel:

  "kernelspec": {
   "display_name": "Python 3",
   "language": "python",
   "name": "python3"
  },

If you can switch it in the notebook while we fixe the tooling, that'd be 👍

@@ -0,0 +1,640 @@
{
Copy link
Member

@kelle kelle Dec 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, no other tutorials have an Index like this. I'd like to discuss this.


Reply via ReviewNB

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this and decided to handle the Index as part of the template so every tutorial will have it automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also highlighted this as a nice low-hanging fruit issue to tackle in e.g. astropy/learn-astropy#26

@keflavich keflavich force-pushed the spectral-cube-reprojection branch from e479c64 to 4364bda Compare January 4, 2022 17:30
@keflavich keflavich force-pushed the spectral-cube-reprojection branch from 4364bda to 36d813c Compare January 18, 2022 13:54
@kelle kelle self-requested a review January 20, 2022 15:21
@keflavich keflavich force-pushed the spectral-cube-reprojection branch from 2f321a6 to 021e53b Compare January 20, 2022 15:55
@jonathansick
Copy link
Contributor

In GitHub Actions, we got a network timeout downloading a file:

filename_1 = download_file("https://almascience.nrao.edu/dataPortal/member.uid___A001_X1465_X3a33.BrickMaser_sci.spw71.cube.I.manual.image.pbcor.fits",

I'll re-run the GitHub Actions job now to see if it clears (if it persists we might want to look into a retry loop? or loading the files from somewhere else?)

@keflavich
Copy link
Contributor Author

unfortunately, that error is expected. NRAO's ALMA server has been timing out on me all week.

We can try the ESO and Japan mirrors, but a retry loop is the most reliable. It's super clunky, though.

@jonathansick
Copy link
Contributor

Good news it looks like the re-run of the GHA workflow worked!

We can try the ESO and Japan mirrors, but a retry loop is the most reliable. It's super clunky, though.

On one hand, we could think about adding a retry-loop (with exponential back-offs and all that) to astropy.utils.data.download_file or a wrapper of it. Until we start caching built tutorials, so we don't need to rely on every tutorial being runnable all the time, either a retry or just caching to a more solid static file server is our best bet.

Since it's working right now, I think we can keep going, but it's something we might need to come back to soon.

@eblur
Copy link
Contributor

eblur commented Jan 27, 2022

This looks great! I had a few minor comments -- mainly to do with adding some exposition around the science context for performing these tasks. Adding that context will greatly aid in the search discovery potential for this tutorial.

@kelle
Copy link
Member

kelle commented Jan 27, 2022

I'm confused. It looks like the build is failing. @jonathansick , are you saying it can be merged anyway?

@jonathansick
Copy link
Contributor

It was working, but not now. In this case, it looks like the server really is wonky and we should find a work-around for the download issue before merging.

@keflavich keflavich force-pushed the spectral-cube-reprojection branch from 165b84d to 6287f1d Compare February 17, 2022 15:25
@kelle
Copy link
Member

kelle commented Feb 24, 2022

Maybe we should serve the FITS file from the astropy data server instead of downloading from the ALMA server.

@keflavich
Copy link
Contributor Author

I'd be happy with that; I just picked data that were publicly available b/c I wasn't sure how much we could afford to put on the astropy server

@@ -0,0 +1,651 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #1.    filename_1 = download_file("https://almascience.nrao.edu/dataPortal/member.uid___A001_X1465_X3a33.BrickMaser_sci.spw71.cube.I.manual.image.pbcor.fits",

Maybe this should use astroquery.alma instead? Might that be more reliable than the direct download?


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it will not. The archive is the bottleneck, not the access path to the archive.

@jonathansick
Copy link
Contributor

jonathansick commented Mar 31, 2022

@keflavich I can't download the datasets at all:

(500 server errors)

Are there different URLs we can try? Once we download the datasets I'll add them to the Astropy data server for more stable access.

@jonathansick
Copy link
Contributor

Oh nevermind; the eso.org archive works. Maybe we can change the links to avoid using the archive server?

@jonathansick jonathansick merged commit f9a2691 into astropy:main Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants