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

Changes to the FITS-header Tutorial #559

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

saimafsiddiqui
Copy link

@saimafsiddiqui saimafsiddiqui commented May 29, 2022

  • Check the box to confirm that you are familiar with the contributing guidelines and/or indicate (check the box) that you are familiar with our contributing workflow.
  • Confirm that any contributed tutorials contain a complete Introduction which includes an Author list, Learning Goals, Keywords, Companion Content (if applicable), and a Summary.
  • Check the box to confirm that you are familiar with the Astropy community code of conduct and you agree to follow the CoC.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@eblur eblur requested a review from mwcraig June 8, 2022 18:10
Copy link
Member

@mwcraig mwcraig left a comment

Choose a reason for hiding this comment

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

Hi 👋 @saimafsiddiqui and thanks for this pull request! It is good to have people taking a fresh look at these tutorials.

I have several suggestion below -- mostly very minor. FITS is a weird file format, in that it is strict in some ways (keywords can have no more than 8 characters) but very loose in others (images can be in any HDU), so I tried to note some cases where you will want to be a little less broad in how you talk about fits.

Thanks again for working on this!

tutorials/FITS-header/FITS-header.ipynb Outdated Show resolved Hide resolved
"id": "qkFwDQUCmv2Y"
},
"source": [
"Most of the time, the convenience functions are the easiest way to access the data. `fits.getdata()` reads only the data from a FITS file, but with the `header=True` keyword argument will also read the header. "
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I agree that this is the easiest -- if you just need the header or data alone and don't need to modify the file, then maybe it is more convenient. If you want to change the file, though, I think fits.open is probably preferable.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you! I'll change how I wrote that and add a little more about the functions for clarity.

tutorials/FITS-header/FITS-header.ipynb Outdated Show resolved Hide resolved
tutorials/FITS-header/FITS-header.ipynb Outdated Show resolved Hide resolved
tutorials/FITS-header/FITS-header.ipynb Outdated Show resolved Hide resolved
tutorials/FITS-header/FITS-header.ipynb Outdated Show resolved Hide resolved
tutorials/FITS-header/FITS-header.ipynb Outdated Show resolved Hide resolved
tutorials/FITS-header/FITS-header.ipynb Outdated Show resolved Hide resolved
tutorials/FITS-header/FITS-header.ipynb Outdated Show resolved Hide resolved
tutorials/FITS-header/FITS-header.ipynb Show resolved Hide resolved
fixed quotations

Co-authored-by: Matt Craig <[email protected]>
@eblur eblur requested a review from mwcraig October 5, 2022 18:32
@eblur
Copy link
Contributor

eblur commented Oct 5, 2022

@mwcraig Can you check that your changes were implemented properly? If not, I'll contact Saima so we can upload her updates.

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.

3 participants