Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a module to query NEOs (From Poliastro) #1325

Closed
wants to merge 6 commits into from
Closed

Add a module to query NEOs (From Poliastro) #1325

wants to merge 6 commits into from

Conversation

shreyasbapat
Copy link

Hi!

As discussed on astroquery chats some days ago!
https://matrix.to/#/!DdazxkLDTPnBLekbbL:openastronomy.org/$153784692124yzLpI:openastronomy.org

We had a neos module in our library, poliastro which essentially makes it possible to search NEOs by name (also using wildcards), and get their orbits straight from NASA APIs, using some of the defined methods. We can plot the orbit of a NEOs in just 2-3 lines of code. We thought that contributing the NEOs code to astroquery will be a better idea as this will make it even more usable. See the short documentation of NEOs module here: http://docs.poliastro.space/en/latest/user_guide.html#working-with-neos And API reference here: http://docs.poliastro.space/en/latest/safe.html#module-poliastro.neos

I have added some tests for the same as well, kindly let me know the changes required. :)

cc @Juanlu001

@pep8speaks
Copy link

pep8speaks commented Jan 6, 2019

Hello @shreyasbapat! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 06, 2019 at 07:20 Hours UTC

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

LGTM so far. The tests appear to be remote-only, so they need to be tagged as such, otherwise this looks fine. We'd like some local tests before merging, though.

}
SBDB_URL = conf.sbdb_server
response = self._request(
"GET", SBDB_URL, params=payload, timeout=self.TIMEOUT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to (optionally?) enable cacheing here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, sure. I will do that!


# response = requests.get(SBDB_URL, params=payload)
response.raise_for_status()
# soup = BeautifulSoup(response.text, "html.parser")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# soup = BeautifulSoup(response.text, "html.parser")

Copy link
Author

Choose a reason for hiding this comment

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

I will remove that!

from astropy.table import Table


def test_observations_from_spk_id():
Copy link
Contributor

Choose a reason for hiding this comment

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

These are remote-data tests, right? They need to be tagged with @remote_data if so.

Copy link
Author

Choose a reason for hiding this comment

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

Correct! I will tag them.
Also, I will add some local tests too.

@bsipocz
Copy link
Member

bsipocz commented Jan 8, 2019

We already have the jplsbdb module, so I wonder how much of this functionality complementary of what that module can do. My opinion is that querying a subclass of objects from the generic module should not be a new module itself.

cc @mommermi

@mommermi
Copy link
Contributor

mommermi commented Jan 9, 2019

It seems to me that all the data that can be queried from NEOWS originate from SBDB, so implementing that service would be redundant. I had a quick look at Poliastro and there is a big overlap with what we already have in astroquery (.jplhorizons and .jplsbdb). What would be an interesting new addition is an interface to query orbits from DASTCOM5. @shreyasbapat is that something you are planning to implement here?

@shreyasbapat
Copy link
Author

Thankyou @mommermi , @bsipocz and @keflavich for prompt replies :)
Yes, I am planning to add another PR for DASTCOM after this!

@shreyasbapat
Copy link
Author

Also, @mommermi , should I fix the issues pointed out by @keflavich or I should perhaps start working on the DASTCOM5 PR and close this?

@keflavich
Copy link
Contributor

@mommermi I noticed you 👍'd the last comment by @shreyasbapat, but he asked an 'or' question. Could you clarify in text what you thing the right way forward is? i.e., should we close this PR?

@mommermi
Copy link
Contributor

@keflavich I guess it's up to you whether you want to close this PR. To me it sounds like most of the functionality here is already present in jplhorizons or jplsbdb. @shreyasbapat is there anything in this PR that is not yet available in astroquery? (Except for the Dastcom5 module)

@shreyasbapat
Copy link
Author

It queries Neows service provided by JPL :). I don't know until what extent it coincides with SBDB. Yes, a small part of the PR also queries SBDB. If you think it does not have a lot of application, I can happily close it. I am already working on DASTCOM5 PR as you asked me to do so.

@bsipocz
Copy link
Member

bsipocz commented Jan 30, 2019

To me it still looks like it only wraps a convenience service that is actually covered by the current functionalities, so I'm going ahead and close this issue.

Thanks @shreyasbapat for raising this issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants