-
-
Notifications
You must be signed in to change notification settings - Fork 409
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
Accept MAST URIs as input to get_cloud_uris() #3193
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3193 +/- ##
==========================================
+ Coverage 68.34% 68.69% +0.34%
==========================================
Files 231 231
Lines 19190 19214 +24
==========================================
+ Hits 13116 13199 +83
+ Misses 6074 6015 -59 ☔ View full report in Codecov by Sentry. |
Tagging @bmorris3 and @dr-rodriguez for visibility |
@@ -814,7 +804,29 @@ def test_get_cloud_uris(self, test_obs_id): | |||
Observations.get_cloud_uris(products, | |||
extension='png') | |||
|
|||
def test_get_cloud_uris_query(self): | |||
def test_observations_get_cloud_uris_list_input(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test triggers a datetime deprecationwarning, could you investigate which upstream package it comes from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to reproduce a warning, but I did notice a bug in that test when running it alone that may have been causing some issues? I pushed a fix for it, and hopefully that should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still see this:
_________________________________ TestMast.test_observations_get_cloud_uris_list_input _________________________________
self = <astroquery.mast.tests.test_mast_remote.TestMast object at 0x12c775850>
def test_observations_get_cloud_uris_list_input(self):
pytest.importorskip("boto3")
uri_list = ['mast:HST/product/u24r0102t_c1f.fits',
'mast:PS1/product/rings.v3.skycell.1334.061.stk.r.unconv.exp.fits']
expected = ['s3://stpubdata/hst/public/u24r/u24r0102t/u24r0102t_c1f.fits',
's3://stpubdata/panstarrs/ps1/public/rings.v3.skycell/1334/061/rings.v3.skycell.1334.'
'061.stk.r.unconv.exp.fits']
# enable access to public AWS S3 bucket
Observations.enable_cloud_dataset()
# list of URI strings as input
uris = Observations.get_cloud_uris(uri_list)
assert len(uris) > 0, f'Products for URI list {uri_list} were not found in the cloud.'
assert uris == expected
# check for warning if filters are provided with list input
> with pytest.warns(InputWarning, match='Filtering is not supported'):
E DeprecationWarning: datetime.datetime.utcnow() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.datetime.now(datetime.UTC).
../../.tox/test-online/lib/python3.12/site-packages/astroquery/mast/tests/test_mast_remote.py:824: DeprecationWarning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind sending the package versions you're using? I can't reproduce, and I figure I might be running the test with a newer version of some dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is with a fresh tox run, so I'm surprised you don't hit the same problem.
Anyway, it's not in our own codebase, so can just put a blanket ignore in the config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that might be the issue: I've been running the tests with pytest. I can't quite figure out how to use tox to only run the tests in one file. I've been trying to use tox -e py312-test-alldeps-online -- astroquery/mast/tests/test_mast_remote.py
but I keep getting various errors.
146daaf
to
5da743b
Compare
5da743b
to
60cc19d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor issue with the changelog and the warning filter, the rest looks good to me.
Thanks!
CHANGES.rst
Outdated
mast | ||
^^^^ | ||
|
||
- Handle coordinates that are not in the ICRS frame in query functions. [#3164] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be here
setup.cfg
Outdated
# Datetime deprecation warnings | ||
ignore::DeprecationWarning:datetime\.datetime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work as is, besides it's better to be as specific as possible rather than ignoring anything in datetime.datetime
# Datetime deprecation warnings | |
ignore::DeprecationWarning:datetime\.datetime | |
# Triggered in mast, likely boto related | |
ignore:datetime.datetime.utcnow\(\) is deprecated:DeprecationWarning |
Sorry for the delay on this! I just made those changes. |
Thanks @snbianco! |
mast | ||
^^^^ | ||
|
||
- Handle a MAST URI string as input for ``Observations.get_cloud_uri`` and a list of MAST URIs as input for | ||
``Observations.get_cloud_uris``. [#3193] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi: I'll move this section down below as we keep this section to highlight potentially breaking API changes rather than for additions/enhacements.
This PR does the following:
Observations.get_cloud_uri
.Observations.get_cloud_uris
. Logs a warning if the user tries to provide filter criteria as well, since this won't be possible without aTable
.MastMissions
.