Skip to content

Fix bug in the frame attributes references for Galactocentric frame#9582

Merged
eteq merged 2 commits intoastropy:masterfrom
adrn:coordinates/fix-galcen-refs
Nov 12, 2019
Merged

Fix bug in the frame attributes references for Galactocentric frame#9582
eteq merged 2 commits intoastropy:masterfrom
adrn:coordinates/fix-galcen-refs

Conversation

@adrn
Copy link
Member

@adrn adrn commented Nov 11, 2019

Description

While working on astrofrog#91 to write a what's new entry for the changes to Galactocentric, I noticed that the dictionary of references (i.e. scientific papers that the parameter values come from) was not being properly handled. This adjusts the logic to fix how references are stored, and adds a test.

@pllim pllim added Bug Affects-dev PRs and issues that do not impact an existing Astropy release labels Nov 11, 2019
@pllim pllim added this to the v4.0 milestone Nov 11, 2019
@pllim pllim requested a review from eteq November 11, 2019 14:58
@pllim
Copy link
Member

pllim commented Nov 11, 2019

In the spirit of #7928, please run these new tests without ignoring warnings and see if they introduce any new warnings. If so, please handle the warnings accordingly. Thanks!

@adrn
Copy link
Member Author

adrn commented Nov 11, 2019

I copied the setup.cfg from #7928 and ran python setup.py test -t astropy/coordinates locally -- not seeing any new warnings

@adrn
Copy link
Member Author

adrn commented Nov 11, 2019

@pllim Now I did - still no warnings!

@pllim
Copy link
Member

pllim commented Nov 12, 2019

Thanks for checking, @adrn!

@adrn adrn requested review from astrojuanlu and mhvk November 12, 2019 15:02
Copy link
Member

@eteq eteq left a comment

Choose a reason for hiding this comment

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

Mostly good, but one comment that should probably be addressed in some way so that the test doesn't bleed over to other tests accidentally

Copy link
Member

@eteq eteq left a comment

Choose a reason for hiding this comment

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

LGTM now and pretty straightforward, so merging!

@eteq eteq merged commit a89dcba into astropy:master Nov 12, 2019
@adrn adrn mentioned this pull request Nov 12, 2019
18 tasks
# Note: this is necessary because the doctests may alter the state of the
# class globally and simultaneously when the tests are run in parallel
reset_galactocentric_defaults()
# reset_galactocentric_defaults()
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, missed in review...

Copy link
Member

Choose a reason for hiding this comment

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

So is this line supposed to be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, no reason to just comment it out, it is now done in setup and teardown. Indeed, the comment above can go too...

Copy link
Member

Choose a reason for hiding this comment

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

OK, I'll fold it into #7928.

pllim added a commit to pllim/astropy that referenced this pull request Nov 17, 2019
pllim added a commit to pllim/astropy that referenced this pull request Nov 17, 2019
pllim added a commit to pllim/astropy that referenced this pull request Nov 17, 2019
Remove unused code as follow up of astropy#9582

Handle failures cased by numpy/numpy#8945
bsipocz pushed a commit that referenced this pull request Nov 18, 2019
Fix bug in the frame attributes references for Galactocentric frame
maxnoe pushed a commit to maxnoe/astropy that referenced this pull request Jan 6, 2020
Remove unused code as follow up of astropy#9582

Handle failures cased by numpy/numpy#8945
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release Bug coordinates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants