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

Added rsun_obs property to return a quantity #3099

Merged
merged 4 commits into from
May 14, 2019
Merged

Conversation

MSKirk
Copy link
Member

@MSKirk MSKirk commented May 9, 2019

Description

Added rsun_obs property to stereo map sources. It returns a quantity in arcsec consistent with other Maps and overwrites mapbase's assumption of a photospheric limb as seen from Earth.

@pep8speaks
Copy link

pep8speaks commented May 9, 2019

Hello @MSKirk! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 37:1: E302 expected 2 blank lines, found 1
Line 41:1: E302 expected 2 blank lines, found 1
Line 45:101: E501 line too long (107 > 100 characters)

Comment last updated at 2019-05-14 14:21:41 UTC

@ghost
Copy link

ghost commented May 9, 2019

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

@nabobalis nabobalis added this to the 1.0 milestone May 9, 2019
@nabobalis nabobalis added the map Affects the map submodule label May 9, 2019
@nabobalis
Copy link
Contributor

Any chance of a test and a changelog entry?

@codecov
Copy link

codecov bot commented May 9, 2019

Codecov Report

Merging #3099 into master will decrease coverage by 0.1%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3099      +/-   ##
==========================================
- Coverage   89.96%   89.86%   -0.11%     
==========================================
  Files         148      148              
  Lines       10557    10564       +7     
==========================================
- Hits         9498     9493       -5     
- Misses       1059     1071      +12
Impacted Files Coverage Δ
sunpy/map/sources/stereo.py 83.01% <28.57%> (-8.29%) ⬇️
sunpy/map/map_factory.py 91.6% <0%> (-1.4%) ⬇️
sunpy/net/helioviewer.py 93.25% <0%> (-1.13%) ⬇️
sunpy/instr/fermi.py 88.57% <0%> (-0.72%) ⬇️
sunpy/net/attr.py 88.54% <0%> (-0.45%) ⬇️
sunpy/database/tables.py 92.15% <0%> (-0.35%) ⬇️
sunpy/database/database.py 95.75% <0%> (-0.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8aa10a3...059b618. Read the comment docs.

@MSKirk
Copy link
Member Author

MSKirk commented May 9, 2019

Done.

@nabobalis
Copy link
Contributor

Thanks!

@wafels
Copy link
Member

wafels commented May 10, 2019

This has been missing for a long time. Ugh. Thanks for fixing this @MSKirk.

As a general comment, it might be worth someone's time to go through all our other map sources to find out which properties fall back on defaults, and then to find out if this is justified.

@nabobalis
Copy link
Contributor

Conda build failure is unrelated to the PR.

sunpy/map/sources/stereo.py Outdated Show resolved Hide resolved
changelog/3099.bugfix.rst Outdated Show resolved Hide resolved
@Cadair Cadair merged commit 0ee3b86 into sunpy:master May 14, 2019
@MSKirk MSKirk deleted the EUVI_Map branch May 14, 2019 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
map Affects the map submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants