NOIRLab advanced search #1701
Open
NOIRLab advanced search #1701
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Milestone needs to be set on this (0.4.1). If its possible for me to do it, let me know how. |
Codecov Report
@@ Coverage Diff @@
## master #1701 +/- ##
==========================================
+ Coverage 62.67% 62.97% +0.29%
==========================================
Files 189 195 +6
Lines 15192 15295 +103
==========================================
+ Hits 9522 9632 +110
+ Misses 5670 5663 -7
Continue to review full report at Codecov.
|
|
I don't think the documentation is complete. I must have lost something in the shuffle of three PRs plus one previous version of this. Looking into it. (It should have lots of extra methods in the API) |
bsipocz
reviewed
Apr 23, 2020
|
@pothiers - It would be nice to have more substantial user facing documentation, with more examples. If you prefer it can be added in a follow-up PR once this is merged. E.g. the nice test coverage could be worked into a narrative docs, too. Also, I see 3 failures that we won't see on CI as we don't run the remote tests for PRs to limit the stress on remote servers. For the record these are the failures I see. Note for @keflavich and @ceb8 - we need to double check the test locally before merging this. |
Have added more "advanced search" documentation. |
When I did: I got no errors. But did get them when doing: I thought the two would be equivalent for module only checks. I'll change my checklist to only use the latter test invocation. |
|
Because there where changes to my PR1701 I merged it locally. I now think I should have rebased. I haven't yet pushed my updates (including conflict resolution). Do I need to somehow untangle to avoid the extra commit logs that will otherwise show? If so, I'll likely need help with the untangling. |
Failed tests were due to new release of our server that happen to come after I submitted working tests. Fixed locally but awaiting guidance on how to best combine changes to PR |
|
@pothiers yes, let's rebase this and try to disentangle the histories. Ping me on slack if you need help |
|
@keflavich - git history-wise this is all right now |
bsipocz
reviewed
Apr 29, 2020
|
CI has 2 test failing with astropy-dev due to this: astropy/astropy#10227, thus it shouldn't be a blocker for this PR. However the async/sync issue should still be addressed before merging. |
Thank you. What I had in mind is to have a bit more exact examples, driven by simple science cases to showcase the functionality of the module (not necessarily just the advanced search), otherwise I'm afraid the users won't know what the module is capable of. |
|
I'm getting two failures when I run the remote tests, both the same:
|
Co-authored-by: Brigitta Sipőcz <[email protected]>
keflavich
reviewed
May 14, 2020
astroquery/noirlab/core.py
Outdated
Comment on lines
182
to
187
| def aux_fields(self, instrument, proctype, cache=True): | ||
| """List the available AUX fields. AUX fields are ANY fields in the | ||
| Archive FITS files that are not core DB fields. These are generally | ||
| common to a single Instrument, Proctype combination. AUX fields are | ||
| slower to search than CORE fields. """ | ||
| url = f'{self._adsa_url}/{instrument}/{proctype}/' |
There was a problem hiding this comment.
since instrument and proctype are user-entered parameters, please document them here (and maybe consider adding some sort of validity check)
There was a problem hiding this comment.
All of these functions are supported by web-services which do the validation. We have people accessing directly via the web-services. Doing validation here would be redundant (and require DB access since what is valid depends on DB. I've added note in doc that says the acceptable values come from CATEGORICALS method.
There was a problem hiding this comment.
We need the parameters documented here, in the docstring.
If a function takes parameters, as this one does, those parameters need to be documented in the docstring, i.e.:
Parameters
----------
instrument : str
Instrument name, see <categoricals?> for details
etc.
| return response.json() | ||
|
|
||
| @class_or_instance | ||
| def query_metadata(self, qspec, limit=1000, cache=True): |
There was a problem hiding this comment.
this could also be async'd, but nbd if it isn't
it does, however, need some docs!
There was a problem hiding this comment.
I mean, this function requires a docstring.
| response.raise_for_status() | ||
| return astropy.table.Table(rows=response.json()) | ||
|
|
||
| def retrieve(self, fileid, cache=True): |
|
The idea behind
In the current version of this code, |
keflavich
reviewed
May 18, 2020
Comment on lines
103
to
109
| self._validate_version() | ||
| ra, dec = coordinate.to_string('decimal').split() | ||
| url = f'{self.url}?POS={ra},{dec}&SIZE={radius}&format=json' | ||
| url = f'{self.siaurl}?POS={ra},{dec}&SIZE={radius}&format=json' | ||
| response = self._request('GET', url, | ||
| timeout=self.TIMEOUT, | ||
| cache=cache) | ||
| response.raise_for_status() |
There was a problem hiding this comment.
Suggest replacing all of this with:
response = self.query_region_async(coordinate, radius=radius, cache=cache)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds capability to search ANY metadata in Archived FITS file headers (in any HDU).