Skip to content

Expand SkyCoord.guess_from_table - #11417

Merged
adrn merged 1 commit into
astropy:masterfrom
nstarman:SC_guess_from_table_fix
Apr 3, 2021
Merged

Expand SkyCoord.guess_from_table#11417
adrn merged 1 commit into
astropy:masterfrom
nstarman:SC_guess_from_table_fix

Conversation

@nstarman

@nstarman nstarman commented Mar 24, 2021 •
edited by pllim
Loading

Copy link
Copy Markdown
Member

Description

  1. Allow for multiple regex matches when they are exact component names.
    eg: allow for column “dec” and “pm_dec” to work
  2. allow for differential component names
    eg: "pm_ra_cosdec", “pm_dec”
  3. improve error message to show all matches, if there are more than one.
  • write tests
  • changelog
  • docstring
  • docs (?)

Signed-off-by: Nathaniel Starkman (@nstarman) [email protected]

@pep8speaks

pep8speaks commented Mar 24, 2021 •
edited
Loading

Copy link
Copy Markdown

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

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-04-03 06:35:45 UTC

@pllim pllim added this to the v4.3 milestone Mar 24, 2021
@nstarman
nstarman force-pushed the SC_guess_from_table_fix branch 7 times, most recently from dab8d3b to cb83b03 Compare March 25, 2021 03:01
@adrn
adrn self-requested a review March 25, 2021 12:37
eteq
eteq approved these changes Mar 25, 2021
View reviewed changes

@eteq eteq left a comment •
edited
Loading

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.

Thanks @nstarman - My initial thought on seeing this was "feature creep!" since this method is supposed to be limited in scope. but now that I look at it I think this is good because it really is very logical and unambiguous! Just one minor wording suggestion.

Comment thread astropy/coordinates/sky_coordinate.py Outdated
@nstarman

nstarman commented Mar 25, 2021

Copy link
Copy Markdown
Member Author

"feature creep!" since this method is supposed to be limited in scope.

@eteq, I refrained from further modifications, but would be interested in pushing more if it's not feature creep. In particular:

  • checking for parallax / redshift if distance isn't found and using coord.Distance()
  • checking for exact component name matches to break ambiguity; e.g. "ra" is preferred over "RA[J2000]" if both are in the table. This might be a flag "prefer_exact_match", or something.

Do you think any of these would make for another good PR?

mhvk
mhvk reviewed Mar 29, 2021
View reviewed changes
Comment thread astropy/coordinates/sky_coordinate.py Outdated
mhvk
mhvk reviewed Mar 29, 2021
View reviewed changes
Comment thread astropy/coordinates/sky_coordinate.py Outdated
mhvk
mhvk reviewed Mar 29, 2021
View reviewed changes
Comment thread astropy/coordinates/sky_coordinate.py Outdated
@nstarman
nstarman force-pushed the SC_guess_from_table_fix branch 3 times, most recently from 394d374 to f1050c4 Compare March 29, 2021 21:35
@nstarman

nstarman commented Mar 29, 2021

Copy link
Copy Markdown
Member Author

@mhvk, @eteq, @adrn. Tests passing (3.7 failure is unrelated).

mhvk
mhvk approved these changes Mar 29, 2021
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.

Only nitpicks left. Since they are really nitpicks, I'll approve. Nice!

Comment thread astropy/coordinates/sky_coordinate.py Outdated
Comment thread astropy/coordinates/sky_coordinate.py Outdated
Comment thread astropy/coordinates/tests/test_sky_coord.py
Comment thread astropy/coordinates/tests/test_sky_coord.py Outdated
Comment thread astropy/coordinates/tests/test_sky_coord.py Outdated
@nstarman
nstarman force-pushed the SC_guess_from_table_fix branch from f1050c4 to 789b422 Compare March 30, 2021 02:14
adrn
adrn approved these changes Mar 30, 2021
View reviewed changes

@adrn adrn 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.

Thanks - this is great!

Looking at the tests, I think we could definitely improve the test cases and some more test tables to check this with. But I can make a new issue for that, and keep the scope of this PR as is.

@adrn adrn added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Mar 30, 2021
@mhvk

mhvk commented Mar 30, 2021

Copy link
Copy Markdown
Contributor

Super! This is a really nice addition and nicely implemented too!

Now the tests have passed, and I'd love to press the merge button, but for some reason it tells me it can only be done with 6 approving reviews!?!?!?!? Anybody any idea what's going on?

@pllim

pllim commented Mar 30, 2021

Copy link
Copy Markdown
Member

Don't merge yet. We're freezing merges until towncrier is in place.

@nstarman
nstarman force-pushed the SC_guess_from_table_fix branch 2 times, most recently from 6f5b0e4 to eeea043 Compare April 3, 2021 04:48
@nstarman

nstarman commented Apr 3, 2021

Copy link
Copy Markdown
Member Author

@adrn @mhvk @eteq. Rebased to fix conflict. Test failures are unrelated. Switched change-log to Towncrier news fragment.

@nstarman @eteq
differentials and multiple matches
78f3394 
- E.g. allow for column “dec” and “pm_dec”
- clarify error message for multiple matches in Table

Co-authored-by: Erik Tollerud <[email protected]>
Co-authored-by: @mhvk
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
nstarman force-pushed the SC_guess_from_table_fix branch from eeea043 to 78f3394 Compare April 3, 2021 06:35
@adrn

adrn commented Apr 3, 2021

Copy link
Copy Markdown
Member

I think the test failures are unrelated (see #11458) but @pllim could confirm?

@pllim

pllim commented Apr 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

What timing. matplotlib dev just got fixed this morning, so everything (that isn't cron) should be green now except mpldev job on CircleCI. Might need a rebase to pick up RTD fix that @nstarman himself put in. 😄

@adrn

adrn commented Apr 3, 2021

Copy link
Copy Markdown
Member

Looks like the RTD fix is in! So, the mpldev failure is an expected failure, and I will merge. Thanks @nstarman !

@adrn
adrn merged commit d4323b5 into astropy:master Apr 3, 2021
@nstarman
nstarman deleted the SC_guess_from_table_fix branch April 3, 2021 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eteq eteq eteq approved these changes
@adrn adrn adrn approved these changes
@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

coordinates Enhancement zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now.

Projects

None yet

Milestone

v4.3

Development

Successfully merging this pull request may close these issues.

6 participants

@nstarman @pep8speaks @mhvk @pllim @adrn @eteq