Las Cumbres Observatory (LCO) Archive Access - #3659
jnation3406 wants to merge 13 commits into
Conversation
keflavich
left a comment
keflavich
left a comment
There was a problem hiding this comment.
This looks like a great addition, thanks! I asked some questions in the docs.
|
I've added a For unbounded queries ( I also think I've addressed your initial review comments. I see some failing checks in the PR - I don't think the failing CI-devtest is related to my module but should I be updating the Changelog as part of this PR?. Please let me know if there is anything else I need to do to get the module accepted. |
keflavich
left a comment
keflavich
left a comment
There was a problem hiding this comment.
@bsipocz this looks good to me. I skimmed through focusing mostly on documentation, but all the pieces are in place and this is one of the most thorough matches to our template documents we've gotten.
I'd recommend we do one more pass for obvious things, but then merge and allow for small corrections in subsequent PRs.
@jnation3406 thanks for an excellent new module!
bsipocz
left a comment
bsipocz
left a comment
There was a problem hiding this comment.
I have a bunch of comments, mostly about API consistency with the rest of astroquery.
Also, as this is a new module, I would recommend of not doing any of the async_to_sync machinery. And that being said, I suppose we need to cleanup the docs, too, in a separate PR.
|
|
||
| #: Filters accepted by the ``/frames/`` endpoint. Anything not in here is | ||
| #: rejected client-side so that typos do not silently return the whole archive. | ||
| FRAME_FILTERS = ( |
There was a problem hiding this comment.
Is there a way to programmatically get this from the serverside? How can we ensure that this stays up-to-date?
There was a problem hiding this comment.
Unfortunately we don't currently have an endpoint that returns the full set of filter options for frame queries. These change very rarely - basically never being removed, only occasionally adding new filter options. I will try to keep this module updated as we do add new options, but the functionality defined in the module should continue to work regardless.
|
|
||
| return table | ||
|
|
||
| def get_frame(self, frame_id, *, cache=None): |
There was a problem hiding this comment.
I suppose we will prefer to call this get_image or get_images as we have that for the other modules, we don't call it "frames" anywhere else.
There was a problem hiding this comment.
So this method just retrieves metadata, including a fresh download url, for a given id from our archive database. I don't see any precedence for what it's doing in other modules, and it's only really meant to be an internal method (called by download_files in the case that the frames table provided has expired image urls, i.e. a few days passed since the query was made before they tried to download the files). If I rename it to get_metadata would that satisfy your concerns?
There was a problem hiding this comment.
or I could name it _get_metadata so users are less likely to try to use it themselves and get confused?
| return self._aggregate('proposals') | ||
|
|
||
|
|
||
| class _ProgressBarOrCountingSpinner(ProgressBarOrSpinner): |
There was a problem hiding this comment.
This subclass overrides the behavior of the default Spinner provided by astropy. The default only spins indefinitely for a query with limit=-1 (unbounded). This adds a count next to the spinner of the amount of images currently retrieved (since we batch up retrievals per call of 100 or 1000). That way the user can see there is progress being made on their query, and approximately what rate data is coming back at.
|
@bsipocz Thank you for your review comments. I think I've addressed them all in my last commit, but please let me know if there is anything else you would like me to change. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3659 +/- ##
==========================================
+ Coverage 73.42% 73.95% +0.53%
==========================================
Files 230 232 +2
Lines 21360 21835 +475
==========================================
+ Hits 15684 16149 +465
- Misses 5676 5686 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… the frames, taken from the frames polygon area.
This branch has not been deployed
3 participants
This is an updated module for Las Cumbres Observatory (LCO)'s Archive. I tried to follow the template and existing modules examples for everything, but please let me know if there are any API changes that need to be made.
One question I have is about the use of
async. I decorated the class with@async_to_syncas shown in the template example, but after reading #2598, I am wondering if I should be leaving that out completely? The LCO Archive API is entirely synchronous in its request/response cycle - it does not support asynchronous jobs since its just querying and returning data.There are unit tests, remote tests, and documentation for the module - please let me know if anything else is needed.