Skip to content

Add Astrometry.net support#521

Closed
keflavich wants to merge 17 commits intoastropy:masterfrom
fred3m:astrometry_net
Closed

Add Astrometry.net support#521
keflavich wants to merge 17 commits intoastropy:masterfrom
fred3m:astrometry_net

Conversation

@keflavich
Copy link
Copy Markdown
Contributor

Working on this with Victor Terron

@keflavich
Copy link
Copy Markdown
Contributor

@fred3m - could you link to the WIP branch?

@fred3m
Copy link
Copy Markdown
Contributor Author

fred3m commented Apr 24, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could we call this AstrometryNetClass instead?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sure, I'll rename it.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.46%) to 64.4% when pulling 66013dd on fred3m:astrometry_net into b9c6ae6 on astropy:master.

@vterron
Copy link
Copy Markdown

vterron commented Apr 24, 2015

I have talked to the Astropy developers and it seems that automatically writing the API in the configuration file would not be a good idea. There are several reasons for this — the most convincing of them probably being that this could be very confusing for users, who may found that another library has modified their configuration file by calling our code.

Would using getpass be an option? The Astronometry.net API key does not have an associated username, so maybe we could use something like astrometry-net-user as a placeholder.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.51%) to 64.35% when pulling 52da66d on fred3m:astrometry_net into b9c6ae6 on astropy:master.

@fred3m
Copy link
Copy Markdown
Contributor Author

fred3m commented Apr 25, 2015

Ok, I think this should be good to go. Try testing it and let me know if it works. The only thing that will need to be changed is that they use a print function in a way I've never seen to build their data packet, and it won't be compatible with Python 3 (I had to comment out the future import).

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.45%) to 64.41% when pulling ce2771a on fred3m:astrometry_net into b9c6ae6 on astropy:master.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think you can use self._fp.write instead of print

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.46%) to 64.4% when pulling 6b3095b on fred3m:astrometry_net into b9c6ae6 on astropy:master.

@sargas
Copy link
Copy Markdown
Contributor

sargas commented Aug 10, 2015

Does this code still work? Is there any remaining issues that prevent it from being merged?

@keflavich
Copy link
Copy Markdown
Contributor

Actually, it probably can be merged... it would be nice to have some remote tests in place so we can be sure, but really I just lost track of it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this header needs to be filled out

@fred3m
Copy link
Copy Markdown
Contributor Author

fred3m commented Aug 11, 2015

Sorry, I let this slip through the cracks too. I haven't taken a look at this in a while so it may take me a few days to make the changes.

@keflavich
Copy link
Copy Markdown
Contributor

@fred3m @crawfordsm Just a ping reminding you that this is open and nearly done

@fred3m
Copy link
Copy Markdown
Contributor Author

fred3m commented Dec 2, 2015

Thanks for the reminder. I've been neck deep in job applications and my thesis lately but one weekend when I need a break I'll try and pound this out. Sorry that this is taking so long...

@crawfordsm
Copy link
Copy Markdown
Member

Thanks Fred and good luck with everything!

@mwcraig mwcraig mentioned this pull request Jun 7, 2018
1 task
@bsipocz bsipocz force-pushed the master branch 2 times, most recently from ab0c591 to 712c326 Compare June 8, 2018 12:58
@keflavich
Copy link
Copy Markdown
Contributor

superceded by #1163

@keflavich keflavich closed this Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants