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

Fix two *small* errors in header helper #3184

Merged
merged 7 commits into from Jun 7, 2019

Conversation

Projects
None yet
4 participants
@Cadair
Copy link
Member

commented Jun 3, 2019

Fixes #3179

@sunpy-bot

This comment has been minimized.

Copy link

commented Jun 3, 2019

Thanks for the pull request @Cadair! Everything looks great!

@Cadair Cadair added this to the 1.0.1 milestone Jun 3, 2019

@Cadair Cadair added the map label Jun 3, 2019

@Cadair Cadair force-pushed the Cadair:fix_rsun_header branch from cde6ade to f08f91a Jun 3, 2019

@nabobalis nabobalis requested review from ehsteve and hayesla Jun 3, 2019

meta_wcs['crpix1'], meta_wcs['crpix2'] = (reference_pixel[0].to_value(u.pixel),
reference_pixel[1].to_value(u.pixel))
meta_wcs['crpix1'], meta_wcs['crpix2'] = (reference_pixel[0].to_value(u.pixel) + 1,
reference_pixel[1].to_value(u.pixel) + 1)

This comment has been minimized.

Copy link
@hayesla

hayesla Jun 3, 2019

Contributor

what if someone assumes this already when they add in what the reference pixel should be?

This comment has been minimized.

Copy link
@Cadair

Cadair Jun 3, 2019

Author Member

We need to document this clearer if we expect this to be 1 indexed or 0 indexed. We definitely need to fix it if the user doesn't specify it. I am not sure however if we should expect the user to give us these values 0 or 1 indexed.

This is the only kwarg we need to make this decision on though.

@ayshih
Copy link
Contributor

left a comment

The doctring description for reference_pixel (specifically the default) implies that the input should be 1 indexed. My preference is that the input be 0 indexed – because that's consistent with how the data array is provided – so I say let's update the docstring to reflect that and be explicit about the indexing.

@nabobalis
Copy link
Contributor

left a comment

Tests broke.

Show resolved Hide resolved sunpy/map/header_helper.py Outdated
@ayshih

ayshih approved these changes Jun 7, 2019

@nabobalis nabobalis force-pushed the Cadair:fix_rsun_header branch from 50e66a9 to 9c77f63 Jun 7, 2019

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Rebased, fixed the doc test issues and snuck in a doc dev update.

stale

@nabobalis nabobalis merged commit e8bbe09 into sunpy:master Jun 7, 2019

16 checks passed

ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: pip-install Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 79.83%)
Details
codecov/project 90.36% (+10.52%) compared to d8d5fd2
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy Build #20190607.9 succeeded
Details
sunpy.sunpy (Linux_36_Conda_offline) Linux_36_Conda_offline succeeded
Details
sunpy.sunpy (Linux_36_online) Linux_36_online succeeded
Details
sunpy.sunpy (Linux_37_offline) Linux_37_offline succeeded
Details
sunpy.sunpy (Windows_36_offline) Windows_36_offline succeeded
Details
sunpy.sunpy (macOS_37_offline) macOS_37_offline succeeded
Details
@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Thanks @Cadair

nabobalis added a commit to nabobalis/sunpy that referenced this pull request Jun 7, 2019

Merge pull request sunpy#3184 from Cadair/fix_rsun_header
Fix two *small* errors in header helper

nabobalis added a commit to nabobalis/sunpy that referenced this pull request Jun 7, 2019

Merge pull request sunpy#3184 from Cadair/fix_rsun_header
Fix two *small* errors in header helper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.