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 Celestial Coords 1 tutorial notebook [new content] #453

Merged
merged 6 commits into from
Sep 16, 2020
Merged

Add Celestial Coords 1 tutorial notebook [new content] #453

merged 6 commits into from
Sep 16, 2020

Conversation

kakirastern
Copy link
Contributor

Fixes #443.

Fixes #436 due to a new bug in GitHub.

Address #400, though not completely fixing it.

Add new FITS World Coordinate System (WCS) tutorial to guide users on how to building one's own astropy.wcs.WCS object, serving as an intro to WCS using astropy in two ways programmatically. In addition, a simple way to add a scale bar with the matplotlib package has been introduced.

@kakirastern
Copy link
Contributor Author

@eblur @kelle @stargaser Due to some technical issues I needed to open a new PR for the 1st Celestial Coords tutorial. Will follow up here instead moving forward.

@kakirastern
Copy link
Contributor Author

All code pushed to repo. PR is ready for a review.

@adrn adrn self-requested a review July 29, 2020 14:39
@adrn
Copy link
Member

adrn commented Jul 29, 2020

One quick comment: I would suggest renaming this to "Celestial Coordinates with WCS 1: ...". Taking a look at the infrastructure aspects now!

Copy link
Member

@adrn adrn left a comment

Choose a reason for hiding this comment

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

Looks good to me from an infrastructure perspective!

But: for consistency with other notebooks, could you merge the first cell (the title) with the next cell (header info / authors). You can also remove the "**" (bold), because the header will be bold by default.

@kakirastern
Copy link
Contributor Author

Thanks @adrn Will make the changes suggested by @eblur now

@kakirastern
Copy link
Contributor Author

Looks good to me from an infrastructure perspective!

But: for consistency with other notebooks, could you merge the first cell (the title) with the next cell (header info / authors). You can also remove the "**" (bold), because the header will be bold by default.

Sure, just merged the first two markdown cells, and removed the bolding effect from the headings.

Copy link

@stargaser stargaser left a comment

Choose a reason for hiding this comment

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

I like this version. I've made some suggestions about the file download and access, and the packages in pip-requirements.txt.

pip-requirements.txt Outdated Show resolved Hide resolved
@stargaser
Copy link

For the file download part, it would also work to do from astropy.utils.data import download_file and then do

fname = download_file('https://zenodo.org/record/3946743/files/tailored_dss.22.29.38.50-20.50.13_60arcmin.fits')
hdulist = fits.open(fname)

which is more lines of code than opening the URL directly, or using requests to write a local file. It would have the advantage of not downloading the file every time the user re-executes the notebook. My own feeling is that using download_file is a best practice for Astropy notebooks, but I would like to solicit more opinions on that.

@kakirastern
Copy link
Contributor Author

But I do have reservations about using astropy.utils.data.download_file versus requests when it comes to Zenodo, as it makes the assumption of FITS files being hosted necessarily. Other than that I have no major objections.

@eteq
Copy link
Member

eteq commented Aug 5, 2020

Very minor note: I think the print functions shouldn't be in there in cases where it's at the end of the cell - instead I'd say just have the output of the cell be the string that's being printed.

@kakirastern
Copy link
Contributor Author

Very minor note: I think the print functions shouldn't be in there in cases where it's at the end of the cell - instead I'd say just have the output of the cell be the string that's being printed.

Sure, will make the changes soon.

@kakirastern
Copy link
Contributor Author

Am changing the temporary downloading path to https://github.com/astropy/astropy-data/raw/6d92878d18e970ce6497b70a9253f65c925978bf/tutorials/celestial-coords1/tailored_dss.22.29.38.50-20.50.13_60arcmin.fits before PR is ready to be merged by maintainers.

@kakirastern kakirastern requested review from adrn and eblur September 2, 2020 15:02
@kakirastern
Copy link
Contributor Author

This is ready for another review before the corresponding astropy/astropy-data#95 PR is merged, since the final link from there will be required for maintainers to merge this PR.

Kris Stern and others added 3 commits September 2, 2020 23:38
Update download link for Helix FITS file
Edits to celestial coords 1 tutorial
Copy link
Member

@adrn adrn left a comment

Choose a reason for hiding this comment

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

I did my main review via pull request to @kakirastern's branch here, so I'm happy with the notebook now! One minor remaining thing: I think we can remove requests from the requirements files?

@kakirastern
Copy link
Contributor Author

I did my main review via pull request to @kakirastern's branch here, so I'm happy with the notebook now! One minor remaining thing: I think we can remove requests from the requirements files?

Yup, done 👍

@kakirastern kakirastern requested a review from adrn September 16, 2020 14:39
@kakirastern kakirastern self-assigned this Sep 16, 2020
@adrn adrn merged commit 0292080 into astropy:master Sep 16, 2020
@adrn
Copy link
Member

adrn commented Sep 16, 2020

🎉 Thanks @kakirastern !

@kakirastern
Copy link
Contributor Author

Awesome! Appreciate it @adrn 👍

@kakirastern kakirastern deleted the celestial-coords1 branch September 16, 2020 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add to WCS tutorial: How to place a scale bar on an image with WCS
5 participants