Streamlined method to get list of cloud URIs #3064
Conversation
There was a problem hiding this comment.
Any chance of adding the filters and query criteria as optionals into get_cloud_uris() instead?
Even if it means fully changing that method (as there were already issues with it, see the abandoned PR #2145
Anyway, from the users' POV, I think it would be better to have one method than having multiple with very similar name and functionality.
I agree, that's a great idea. I don't think it should involve a huge rewrite of the method, but I will take a look at that abandoned PR probably sometime next week and see if I can wrap it up. |
|
I moved the logic entirely into |
|
|
||
| # get uris with other functions | ||
| obs = Observations.query_criteria(target_name=234295610, | ||
| provenance_name="SPOC", |
There was a problem hiding this comment.
This triggers a test failure. Could you please run and fix the remote tests?
pytest -P mast -R?
There was a problem hiding this comment.
That's strange, the tests pass on my local machine. What was the error? There's been some slowdowns on the MAST servers recently, so that might be the cause.
Nevermind, I see it now! I was running the wrong command.
|
Ok, the new commit should pass the tests. I also made a few minor changes to the documentation so that those are passing as well. |
There was a problem hiding this comment.
Minor comments about the reason for skipping test for examples. There are a few genuine cases where they can and should be skipping, but for those I would prefer to leave a comment before the example that spells out the reason. So we will remember more easily when next looking into a narrative docs file.
Also, one attribute seems to be undefined, or I just didn't find where it is coming from.
astroquery/mast/cloud.py
Outdated
| s3_path = "s3://{}/{}".format(self.pubdata_bucket, path) | ||
| uri_list.append(s3_path) | ||
| elif full_url: | ||
| path = "http://s3.amazonaws.com/{}/{}".format(self._pubdata_bucket, path) |
There was a problem hiding this comment.
Where is _pubdata_bucket is coming from? I cannot find any definition for it in the repo
There was a problem hiding this comment.
I think it's a typo and meant to be just pubdata_bucket. I'll push a fix!
docs/mast/mast_catalog.rst
Outdated
| @@ -24,7 +24,7 @@ The returned fields vary by catalog, find the field documentation for specific c | |||
| `here <https://mast.stsci.edu/api/v0/pages.html>`__. | |||
| If no catalog is specified, the Hubble Source Catalog will be queried. | |||
|
|
|||
| .. doctest-remote-data:: | |||
| .. doctest-skip:: | |||
There was a problem hiding this comment.
This one was kind of weird, it was failing because the actual output does not match the expected. However, I can't reproduce the results from the tests.
Whenever I run the code on its own, I get the output that's currently in the documentation.
There was a problem hiding this comment.
The new commit doesn't skip the test, but ignores the output of the offending line.
docs/mast/mast_obsquery.rst
Outdated
| @@ -284,7 +284,7 @@ with `~astroquery.mast.ObservationsClass.get_product_list` | |||
| (see `here <https://mast.stsci.edu/api/v0/_c_a_o_mfields.html>`__ for more details). | |||
| Using "obs_id" instead of "obsid" from the previous example will result in the following error: | |||
|
|
|||
| .. doctest-remote-data:: | |||
| .. doctest-skip:: | |||
There was a problem hiding this comment.
why switch to skip for the whole example here?
There was a problem hiding this comment.
Latest commit doesn't skip the test, but ignores the output where the RemoteServiceError occurs.
docs/mast/mast_obsquery.rst
Outdated
| This approach is recommended for code brevity. Query criteria are supplied as keyword arguments, and filters are supplied through the | ||
| ``filter_products`` parameter. If both ``data_products`` and query criteria are provided, ``data_products`` takes precedence. | ||
|
|
||
| .. doctest-skip:: |
There was a problem hiding this comment.
Good point, this shouldn't be skipped. New commit reflects this!
There was a problem hiding this comment.
Looks good now, thanks. I'll go ahead and rebase it before merge to get rid of the merge commit, as our preference is to rebase to pull in upstread changes rather than merge the main back in the feature branch (doing so creates a not too smooth branch structure, especially if doing that multiple time during the development of a feature).
| @@ -286,7 +286,7 @@ Using "obs_id" instead of "obsid" from the previous example will result in the f | |||
|
|
|||
| .. doctest-remote-data:: | |||
| >>> obs_ids = obs_table[0:2]['obs_id'] | |||
| >>> data_products_by_id = Observations.get_product_list(obs_ids) | |||
| >>> data_products_by_id = Observations.get_product_list(obs_ids) # doctest: +IGNORE_OUTPUT | |||
There was a problem hiding this comment.
An expected error can be explicitly tested, too, but nevertheless, this looks good as is.
bsipocz
force-pushed
the
ASB-27903-cloud-uris-from-query
branch
from
cae5372
to
a1349a1
Compare
Added
mast.Observations.get_cloud_uris_query()method so that given a set of query criteria and optional filters, the user receives a list of cloud data URIs for matching data products.Also added to documentation and remote unit tests.