Skip to content

Adopted use of new ERFA ecliptic transformation machinery - #11185

Merged
mhvk merged 1 commit into
astropy:masterfrom
homeboy445:fixfor-4873
Dec 29, 2020
Merged

Adopted use of new ERFA ecliptic transformation machinery#11185
mhvk merged 1 commit into
astropy:masterfrom
homeboy445:fixfor-4873

Conversation

@homeboy445

@homeboy445 homeboy445 commented Dec 22, 2020

Copy link
Copy Markdown
Contributor

Description

Adopted the use of new ERFA ecliptic transformation machinery in ecliptic_transforms.py

Fixes #4873

@homeboy445 homeboy445 changed the title Adoptes use of new ERFA ecliptic transformation machinery Adopted use of new ERFA ecliptic transformation machinery Dec 22, 2020
@pllim
pllim requested review from eteq and mhvk December 22, 2020 14:26
@pllim pllim added this to the v4.3 milestone Dec 22, 2020
@pllim

pllim commented Dec 22, 2020

Copy link
Copy Markdown
Member

Thanks, will need a test and a change log, but before you proceed, let's wait for a review first, as the issue you are trying to fix is very old so I am not sure if it is still an issue or not...

mhvk
mhvk requested changes Dec 22, 2020
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@homeboy445 - thanks for applying the fix. Since @astrojuanlu basically said this was what was needed to be done, no problems there! However, the code can be even simpler, and, more importantly, could you change the comment above the code, otherwise it will just confuse people! Suggestion inline.

Comment thread astropy/coordinates/builtin_frames/ecliptic_transforms.py Resolved
Comment thread astropy/coordinates/builtin_frames/ecliptic_transforms.py Resolved
@pllim

pllim commented Dec 23, 2020

Copy link
Copy Markdown
Member

Thanks for your continued patience, @homeboy445 ! Can you please squash all your commits into one commit, given that there is a merge commit in there anyway?

@pep8speaks

pep8speaks commented Dec 23, 2020

Copy link
Copy Markdown

Hello @homeboy445 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

Line 28:1: E305 expected 2 blank lines after class or function definition, found 0
Line 28:1: E112 expected an indented block
Line 28:2: E999 IndentationError: expected an indented block
Line 32:5: E113 unexpected indentation

astrojuanlu
astrojuanlu approved these changes Dec 29, 2020
View reviewed changes

@astrojuanlu astrojuanlu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me!

mhvk
mhvk approved these changes Dec 29, 2020
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I hadn't noticed the change was already made. All OK now. Thanks, @homeboy445!

@mhvk
mhvk merged commit 196fe57 into astropy:master Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhvk mhvk mhvk approved these changes
@eteq eteq Awaiting requested review from eteq
+1 more reviewer
@astrojuanlu astrojuanlu astrojuanlu approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

coordinates no-changelog-entry-needed Refactoring

Projects

None yet

Milestone

v4.3

Development

Successfully merging this pull request may close these issues.

Use new ERFA ecliptic transformation machinery?

5 participants

@homeboy445 @pllim @pep8speaks @astrojuanlu @mhvk