Access to NOIRLab Astro Data Archive #3359
Conversation
|
TO DO items identified (i.e. before even starting the review process):
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3359 +/- ##
==========================================
+ Coverage 71.73% 71.84% +0.10%
==========================================
Files 235 237 +2
Lines 20258 20354 +96
==========================================
+ Hits 14533 14623 +90
- Misses 5725 5731 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Note to self: |
|
You also need |
bsipocz
left a comment
bsipocz
left a comment
There was a problem hiding this comment.
We did a partial review with some comments, I'm coming back to finish this later this week so no need to address any of these yet.
astroquery/noirlab/core.py
Outdated
| def query_region_async(self, coordinate, radius=0.1, hdu=False, cache=True): | ||
| """Query for NOIRLab observations by region of the sky. | ||
|
|
||
| Given a sky coordinate and radius, returns a `~astropy.table.Table` | ||
| of NOIRLab observations. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| coordinate : :class:`str` or `~astropy.coordinates` object | ||
| The target region which to search. It may be specified as a | ||
| string or as the appropriate `~astropy.coordinates` object. | ||
| radius : :class:`str` or `~astropy.units.Quantity` object, optional | ||
| Default 0.1 degrees. | ||
| The string must be parsable by `~astropy.coordinates.Angle`. The | ||
| appropriate `~astropy.units.Quantity` object from | ||
| `~astropy.units` may also be used. | ||
| hdu : :class:`bool`, optional | ||
| If ``True`` return the URL for HDU-based queries. | ||
| cache : :class:`bool`, optional | ||
| If ``True`` cache the result locally. | ||
|
|
||
| Returns | ||
| ------- | ||
| :class:`~requests.Response` | ||
| Response object. | ||
| """ | ||
| self._validate_version() | ||
| ra, dec = coordinate.to_string('decimal').split() | ||
| url = f'{self._sia_url(hdu=hdu)}?POS={ra},{dec}&SIZE={radius}&VERB=3&format=json' | ||
| response = self._request('GET', url, timeout=self.TIMEOUT, cache=cache) | ||
| # response.raise_for_status() | ||
| return response |
There was a problem hiding this comment.
We may want to have async as a kwarg instead and not have two separate methods. See how we do this in some of the other modules e.g. in cadc or ipac.irsa (or the esa modules).
There was a problem hiding this comment.
As far as I can tell cadc still has the old @async_to_sync setup. I took a look at ipac.irsa, and there it is apparent that the meaning of async_job is to submit asynchronous queries to a job system of some kind. That doesn't apply to the NOIRLab API.
Based on my experience with sdss, historically "async" meant "return the response object from self._request(), while "sync" meant "based on the response object, download the files referenced in it". This is definitely more applicable to NOIRLab.
Can you elaborate further on the different meanings of "async"?
There was a problem hiding this comment.
Based on my experience with sdss, historically "async" meant "return the response object from self._request(), while "sync" meant "based on the response object, download the files referenced in it". This is definitely more applicable to NOIRLab.
This is correct. For the VO case, it can be different, but your interpretation is what I mean by sync vs async.
We have decided that, moving forward, we should not use the async_to_sync functionality, as it is confusing to developers and isn't widely used (#2598). Instead, we prefer kwarg.
It will be a while before we get around to refactoring the old stuff.
There was a problem hiding this comment.
@keflavich is there an example of a module where async_to_sync functionality has been replaced with a keyword?
There was a problem hiding this comment.
yes, modules (e.g. esa, irsa, simbad) do use the async_job kwarg and then connect to the actual async endpoint to the server.
We have decided that, moving forward, we should not use the async_to_sync functionality, as it is confusing to developers and isn't widely used
I would also add that it wasn't real async, the jobs were not run async on the VO service side, and the duplicated methods were not that great from the user POV either.
There was a problem hiding this comment.
@bsipocz, OK, but that's not the case I'm interested in here. I'm referring to "async" methods that would return the raw response rather than doing further conversion into a table or other high-level data. SDSS has several examples of this. See also @keflavich's comment above.
|
@bsipocz, thank you, the suggestions all look good so far, but given my schedule it will be easier to address them all when you're done. |
|
Yes, totally understandable. I really just submitted this half baked review as that is how far we got during the review tutorial during the summer school I was teaching at. I'll get back to the rest later and will write up an actually usable summary, as there are a couple of things that will need to be fixed while the rest is really just some potential follow-up topics. And again, thanks for working on this, getting a working noirlab module will be superb! |
bsipocz
left a comment
bsipocz
left a comment
There was a problem hiding this comment.
I have some more comments to finish up my review.
The overarching theme is to make the API a little bit more consistent with the other modules. And maybe consider using pyvo, but that one is optional for this PR.
And in the meantime it looks like the upstream API has changed, so that may mean some tweaks will be necessary (though the tests are primarily failing with the API number comparison).
Also, the docs build runs into some sphinx errors due to API heading inconsistencies, I'm happy to fix up those before merging.
|
Thank you, I was planning to pick this up next week anyway, so this is good timing. |
|
Commit 64b84bf addresses some of the review comments. Others will be addressed soon. There is an issue that needs to be investigated on the NOIRLab side though. Since I last tested in July, when metadata fields are requested in a query, e.g. |
|
Also, I have no clue why the |
That error rings a bell, and will be related to the generated API docs. I would recommend ignoring it for now and I'll have a closer look once this is ready to go in. |
* main: (115 commits) DOC: removing linking non-module MAINT: addig ignoring for py39 warning triggered in boto DOC: fix whitespacing around references Updates and restructuring of irsa docs. Update parameter definitions in MOST docs. Corrected epoch parameter definition. Further corrections to docstring parameter definitions. changelog Use continuation when downloading files Fix docstring for eccentricity and perihelion time DOC: fixing failing doctest Changelog case insensitive filters Raise error if non-string object names changelog, docstring resolve_objects should accept int DOC: clarification for TAP+ [skip ci] DOC: Remove unused files DOC: Remove defaults channel from RTD update path in remote tests ...
* main: (95 commits) Update change log Add interstellar object and dead comet tests. Rearrange regex comments Allow "dead" comets like 85D Use I for interstellar objects, P for periodic comets. Numbers are not allowed in fragment designations. I/ is not a valid prefix. 1A, 2A, etc. are not valid objects. Update comments to clarify what the code is doing Clarify object reassignment. Bump the actions group with 2 updates maint: switch to the new cds domain name fix: remove message when regions is missing add changes entry remove UCD formatter Address review comments from Sam Bianco enforce upper limit on MastMissions query radius DOC: fix more syntax [skip ci] BUG: fix usage of contentless directives EUCLIDSWRQ-270: Configure response for get_datalinks with metadata option ...
|
@bsipocz, I'm planning to resume work on this PR in the upcoming weeks, in anticipation of a new API version being deployed on the NOIRLab side. One important question above is about There is a less urgent question about whether Other open suggestions are things I just need to look up or otherwise implement. |
* main: (36 commits) Updates to mast_cut GAIAPCR-1368 Include PR reference number Minimal changes to catalogs docs Updated docs Updated main page and mast_query page Bump the actions group with 2 updates changelog Be more robust in getting column configuration GAIAPCR-1368 new retrieval type RESIDUAL_IMAGE fix an error catch codestyle fix cdms remote tests doctest output whitespace fix variable name remove unnecessary text now jplspec / cdms get_molecule behave closer to the same factor out parse_molid move unique sector logic into helper function test coverage for condition when no sectors for target add changelog entry ...
|
@bsipocz, @keflavich, I hope to finish up a few remaining issues with remote tests (which boil down to column ordering in returned data) in the next day or so. There are still two remaining questions from my previous comment. Addressing those should be easy. |
|
PS, all of the test failures on this PR currently are due to items outside of my control. |
|
I solved a documentation warning issue I mentioned way back in July. All remaining documentation warnings appear to be from other packages. The ultimate cause of the warning issue was that automodapi creates a certain header hierarchy (i.e. underline style), and the rest of your document has to be consistent with that, otherwise you get a warning about underlining that doesn't even appear in your document. Once you adjust the underlining to match automodapi the warning goes away. |
|
I haven't tried to run the docs build locally, but these failures look familiar. I would recommend rebasing the whole PR on top of the current |
|
RTD is known to fail and will be fixed separately. |
Thanx. I had already done a recent merge with |
3 participants
This PR closes #1701.
The initial commit simply restores the NOIRLab files to the state and content they were when #1701 was last worked on. Further work and testing will be needed to meet astroquery standards.