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

New module to query Lowell Observatory astorbDB database #3203

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

hhsieh00
Copy link

@hhsieh00 hhsieh00 commented Feb 4, 2025

This is a new module for querying the Lowell Observatory astorbDB database (see issue #3129), specifically using REST APIs used to run their AstInfo tool (https://asteroid.lowell.edu/astinfo/). The AstInfo class in this module specifically retrieves designation, orbital element, orbit, albedo, color, taxonomy, lightcurve, dynamical family, and escape route data for asteroids catalogued in the Lowell astorbDB database.

@bsipocz bsipocz added this to the v0.4.10 milestone Feb 4, 2025
@bsipocz bsipocz marked this pull request as draft February 4, 2025 20:25
@mkelley mkelley self-requested a review February 5, 2025 20:56
Copy link
Contributor

@mkelley mkelley left a comment

Choose a reason for hiding this comment

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

Looks good and very useful in general. Some initial comments and questions to address while you continue testing.


timeout = _config.ConfigItem(
30,
'Time limit for connecting to template_module server.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix "template_module"

_uri = None

def designations_async(self, object_name, *,
get_raw_response=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

get_raw_response keyword argument should not be used (the async method always returns a raw response)

Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, remove the related if statement, and edit the other methods.


def designations_async(self, object_name, *,
get_raw_response=False,
get_uri=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is needed either. Maybe you were following the jplhorizons module? (which is ancient frankencode)

Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, remove the related if statement, and edit the other methods.


return response

def dynamicalfamily_async(self, object_name, *,
Copy link
Contributor

Choose a reason for hiding this comment

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

My personal preference is dynamical_family_async.


return response

def escaperoutes_async(self, object_name, *,
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly: escape_routes_async

"""

if 'elements' in src:
src = OrderedDict(src['elements'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for OrderedDict?

except ValueError:
raise ValueError('Server response not readable.')

if self.query_type == 'designations':
Copy link
Contributor

Choose a reason for hiding this comment

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

if ... elif ... elif would be better: indicates that only one match can occur, and allows for a final else that can raise a ValueError in case of no match.

@bsipocz
Copy link
Member

bsipocz commented Feb 15, 2025

Thanks @mkelley for the review. I haven't looked into it in any meaningful way yet, but I notice that there are a lot of comments on async stuff. We should remove/rework the template module as a lot has changed. E.g. there is a preference of supporting actual async/sync API and not just with the early template hackary. So I would suggest to so that, too, if the service support async, then opt into that with a keyword argument, but don't duplicate the methods with the decorator.

#2598

or one PR that did the keyword approach: #3201 or the ESA modules also have a async_job keyword to switch between sync and async query modes.

@bsipocz bsipocz removed this from the v0.4.10 milestone Mar 18, 2025
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.

3 participants