(re-)Add astrometry.net by mwcraig · Pull Request #1163 · astropy/astroquery

mwcraig · 2018-06-07T01:09:56Z

This PR replaces #521 (but does include all of the commits from that PR with proper attribution). I'll add a test that checks the solution for the small data file I've added later tonight.

The intent is that there also be a solve_from_image method or something that uploads an image for astrometry.net to do the source detection.

I'd like a better way of managing all of the options, but this is a start....

To do

Add API key for astrometry.net

astropy-bot · 2018-06-07T01:09:58Z

Check results are now reported in the status checks at the bottom of this page.

pep8speaks · 2018-06-07T01:09:58Z

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-08-02 01:39:32 UTC

codecov · 2018-06-07T06:19:29Z

Codecov Report

Merging #1163 into master will decrease coverage by 0.14%.
The diff coverage is 52.26%.

@@            Coverage Diff             @@
##           master    #1163      +/-   ##
==========================================
- Coverage   62.61%   62.47%   -0.15%     
==========================================
  Files         174      176       +2     
  Lines       14413    14612     +199     
==========================================
+ Hits         9025     9129     +104     
- Misses       5388     5483      +95

Impacted Files	Coverage Δ
astroquery/astrometry_net/__init__.py	`100% <100%> (ø)`
astroquery/astrometry_net/core.py	`50% <50%> (ø)`

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 2d1ee6f...378360e. Read the comment docs.

keflavich

Initial review. Only minor comments so far.

astroquery/astrometry_net/__init__.py

astroquery/astrometry_net/core.py

astroquery/astrometry_net/tests/test_astrometry_net.py

keflavich · 2018-07-07T18:14:47Z

@mwcraig - just ping us when this is ready for further review

mwcraig · 2018-08-19T21:32:16Z

(still working on this but wanted to push some updates)

mwcraig · 2018-08-21T02:08:53Z

I could use some feedback on how to structure the arguments for the two methods here. The challenge is that each method (solve_from_source_list, solve_from_image) has a few parameters that are specific to the method (e.g. x, y for the source list, name of the image file for the image) and a whole bunch of parameters related to the plate solve that are the same.

Two approaches I can see:

One function that takes the union of all the settings and dispatches the actual solve to internal functions. Advantage: all of the settings are in one place. Disadvantage: messy logic inside this method, potential user confusion about which combinations of parameters they must set.
Two methods with the settings factor out in some way (maybe the settings are properties of the instance? something else?).

I just pushed a version which sort of does the second but I'm not sure I like the approach I took of adding a show_allowed_settings method and an empty_settings property (to make it easier to set up a dict of the options).

keflavich · 2018-08-21T17:13:15Z

I prefer two methods with common settings factored out. Will look at the code too...

keflavich

The approach you've taken seems fine for now, but I haven't seen it in action

astroquery/astrometry_net/core.py

mwcraig · 2018-09-24T01:23:14Z

Still working on the documentation, but the code is just about done...

mwcraig · 2018-09-29T20:11:21Z

@keflavich @bsipocz -- this is ready for review, though there are still a few things to be fixed before merging:

Add an API key for running the remote tests.
Add instructions for adding API key to config and/or keyring, and
add code for retrieving API key from keyring if keyring is used.
Decide whether to change examples to match tests (then users can download sample catalogs/images)
Maybe add RA/Dec to the test for searching by list to speed up the solution.

mwcraig · 2018-09-29T20:14:56Z

@dstndstn -- this PR adds a Python interface to astrometry.net. I have a question -- what is the appropriate rate at which to query the server to see if it is done with a solution? Right now it simply checks every second (see https://github.com/astropy/astroquery/pull/1163/files#diff-93fb4c4b0964e0725414e59df9f4150dR229 ) but I could make it less aggressive. (and thanks again for the service!)

mwcraig · 2018-09-30T16:55:59Z

Question about coverage -- many the bits being reported by codecov as uncovered are actually covered by the remote data tests. Should I be mocking up the remote or something and writing non-remote tests for those things (the methods _login, monitor_submission, solve_from_source_list, solve_from_image)?

Will, of course, write additional tests for everything that clearly does not require remote.

keflavich · 2018-10-01T01:09:38Z

@mwcraig - I will review this if I can find time, but it's a big one and I'm pretty preoccupied. I hope I'll find time this week....

On coverage, we just don't measure the coverage of remote tests, unfortunately.

mwcraig · 2018-10-01T02:05:53Z

No big rush; I'll check the coverage locally with the remote tests running. Let me know if you want me to try mocking up some tests.

bsipocz · 2018-10-01T02:27:10Z

@mwcraig - I'm also snowed it quite badly, but will try to find time to do a review. As for coverage, don't worry that much about mocking for things where local testing is not sensible.
I have high hopes to refactor the CI infrastructure, and then we may hopefully be able to coverage test more jobs etc. that may improve the current situation.

keflavich

minor things only

astroquery/astrometry_net/core.py

keflavich · 2018-12-05T04:09:22Z

astroquery/astrometry_net/core.py

+    def api_key(self):
+        """ Return the Astrometry.net API key. """
+        if not conf.api_key:
+            log.error("Astrometry.net API key not in configuration file")


is the api key required? if so, raise instead of log this error?

An error is raised if you do any operation that requires the api key (all communication with astrometry.net goes through _login, which raises an error). Here the user gets a warning if they try accessing the attribute.

would that be possible to get an API key for testing? We ideally need something for CI cron tests.

Yes; we'd need to set up an account on astrometry.net. I'll do that this morning and pass the credentials off to @bsipocz for CI encryption.

API key has been sent

keflavich · 2018-12-05T04:22:23Z

astroquery/astrometry_net/core.py

+            The contents of the returned object depend on whether the solve
+            succeeds or fails. If the solve succeeds the header with
+            the WCS solution generated by astrometry.net is returned. If the solve
+            fails then an empty dictionary is returned. See below for the outcome


why an empty dict?

I don't remember why I did an empty dict instead of None; happy to change.

astroquery/astrometry_net/core.py

bsipocz · 2018-12-05T07:08:36Z

@mwcraig - Could you rebase one more time to clear up the unrelated test failures?

mwcraig · 2018-12-05T21:07:03Z

@bsipocz -- rebased

These apply to one type of source input (a list) and are not general settings like the rest

…e gracefully

🤦‍♂️

bsipocz added this to the v0.3.9 milestone Jun 7, 2018

bsipocz added the New Service label Jun 7, 2018

keflavich reviewed Jun 7, 2018

View reviewed changes

bsipocz force-pushed the master branch 2 times, most recently from ab0c591 to 712c326 Compare June 8, 2018 12:58

keflavich reviewed Aug 21, 2018

View reviewed changes

astroquery/astrometry_net/core.py Outdated Show resolved Hide resolved

bsipocz self-requested a review September 6, 2018 00:06

mwcraig force-pushed the astrometry_net_update branch from 229e5a9 to 83b5fc7 Compare September 29, 2018 20:04

saimn mentioned this pull request Oct 11, 2018

Switch from coveralls to codecov astropy/astropy#7879

Merged

crawfordsm mentioned this pull request Oct 19, 2018

astrometric solution for the image astropy/ccdproc#160

Open

mwcraig force-pushed the astrometry_net_update branch from 29b0ef4 to ebcd9b6 Compare December 5, 2018 04:12

keflavich requested changes Dec 5, 2018

View reviewed changes

keflavich approved these changes Dec 5, 2018

View reviewed changes

mwcraig force-pushed the astrometry_net_update branch from ce209a5 to 15547d5 Compare December 5, 2018 21:05

mwcraig added 28 commits August 1, 2019 21:39

Remove image_width/image_height from settings

f5da7fa

These apply to one type of source input (a list) and are not general settings like the rest

Include gzipped fits files in test data

df3ab8b

Add solve from image and tests

1feec0b

Remove long docstring from source file

bd15ac7

Fix a PEP8 issue

4d41112

Fix test that fails in Python 2.7

b37f328

Fix broken links in docstrings

0a5d94e

Add narrative docs

86577b6

Add section on handling success, failure and timeouts

ca3a325

Rename method for checking response, expose its API and add test

1233a10

Add changelog entry

057cd30

Remove a few comments left over from the template

34d14f6

Clean up one more line from template

068bcbd

Check whether CCDData is available from astropy and handle its absenc…

2ff64db

…e gracefully

Fix typo in method name

fc4e8ff

Add several tests that do not require remote data

975d250

Use caplog.text instead of .messages which is new-ish

b98d63d

Try skipping a couple tests if pytest is really old

fa5f2f2

Address review comments

868d855

Use LooseVersion instead of a hack

fa4cce6

Fix white space and long lines in docs

f3fd3f2

Fix typo

e72e004

🤦‍♂️

Update checks of error messages to work with pytest >=5

1cc9227

Increase timeout so that remote tests pass

d728080

Add more robust check that test function dependencies are installed

1150a89

Add photutils and scipy as test dependencies

1f9b033

Use ImportError instead of ModuleNotFoundError for py2k compatibility

fedcc82

Add astrometry.net API key for remote_data tests

378360e

mwcraig force-pushed the astrometry_net_update branch from 8d60cce to 378360e Compare August 2, 2019 01:39

keflavich merged commit ec841e2 into astropy:master Aug 2, 2019

Uh oh!

Conversation

mwcraig commented Jun 7, 2018 • edited by keflavich Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To do

Uh oh!

astropy-bot bot commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2019-08-02 01:39:32 UTC

Uh oh!

codecov bot commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

keflavich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keflavich commented Jul 7, 2018

Uh oh!

mwcraig commented Aug 19, 2018

Uh oh!

mwcraig commented Aug 21, 2018

Uh oh!

keflavich commented Aug 21, 2018

Uh oh!

keflavich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mwcraig commented Sep 24, 2018

Uh oh!

mwcraig commented Sep 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mwcraig commented Sep 29, 2018

Uh oh!

mwcraig commented Sep 30, 2018

Uh oh!

keflavich commented Oct 1, 2018

Uh oh!

mwcraig commented Oct 1, 2018

Uh oh!

bsipocz commented Oct 1, 2018

Uh oh!

keflavich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keflavich Dec 5, 2018

Choose a reason for hiding this comment

Uh oh!

mwcraig Dec 5, 2018

Choose a reason for hiding this comment

Uh oh!

bsipocz Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

mwcraig Jul 13, 2019

Choose a reason for hiding this comment

Uh oh!

mwcraig Jul 13, 2019

Choose a reason for hiding this comment

Uh oh!

keflavich Dec 5, 2018

Choose a reason for hiding this comment

Uh oh!

mwcraig Jul 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mwcraig commented Jun 7, 2018 •

edited by keflavich

Loading

astropy-bot bot commented Jun 7, 2018 •

edited

Loading

pep8speaks commented Jun 7, 2018 •

edited

Loading

codecov bot commented Jun 7, 2018 •

edited

Loading

mwcraig commented Sep 29, 2018 •

edited

Loading