MAST: Cutouts limit #2693

jaymedina · 2023-03-29T14:58:32Z

Addresses some points from the conversation here #2447 .

Changes:

Add a warning message for cutout sizes where at least 1 dimension goes beyond the recommended size limit of 30 pixels.
Add a timeout argument to get_cutouts that allows the user to modify the request processing time limit so that at timeout error doesn't occur at 600s.

NOTE: Screenshots are outdated

Users selecting large cutout sizes (either in pixels, degrees, arcmins, or arcseconds) will be met with a warning that they may run into a timeout error. The suggestions are to either change the cutout size to something smaller, or change the request time limit.

The upper limit of 30 pixels (in either direction) is converted to degrees/arcmins/arcseconds for cases where a user may input their cutout size in a unit other than pix:

Users who choose to change the request time limit will receive an INFO message informing them of this change:

Users can also decrease the request time upper limit. Here is an example where we return to default:

Other cutout services remain unaffected:

codecov · 2023-03-29T15:08:32Z

Codecov Report

Merging #2693 (79e6f05) into main (0c06dd5) will decrease coverage by 0.05%.
The diff coverage is 46.87%.

@@            Coverage Diff             @@
##             main    #2693      +/-   ##
==========================================
- Coverage   66.05%   66.01%   -0.05%     
==========================================
  Files         233      233              
  Lines       17925    17953      +28     
==========================================
+ Hits        11840    11851      +11     
- Misses       6085     6102      +17

Impacted Files	Coverage Δ
astroquery/mast/cutouts.py	`64.05% <46.87%> (-2.50%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

bsipocz

I wonder whether the InputWarning is the correct one here, or would rather need a more astroquery/astropy warning type? @keflavich, do you have a preference?

Also, this needs a changelog.

astroquery/mast/cutouts.py

jaymedina · 2023-03-30T17:46:20Z

I believe I've addressed everything now so this is ready for a final review @bsipocz @eerovaher . Once I get this approved, I'll do a squash so it's ready for merge.

astroquery/mast/cutouts.py

bsipocz · 2023-03-31T00:53:29Z

astroquery/mast/cutouts.py

+    def _get_default_timeout(self):
+        """ Gets the default request timeout limit. """
+
+        return self._service_api_connection.TIMEOUT


why is this method necessary? (I see in the history that there has been a bit of a back and forth, so maybe it's not needed any more?

I included this for the case where a user might switch between get_cutouts and download_cutouts but does not anticipate that the timeout setting stayed the same. Having the timeout arg in each individual method implies that it would only change timeout for a call with that method and not any of the others, so I figured it would make sense to keep the changes contained by resetting that arg each time.

What about L414 then? https://github.com/astropy/astroquery/pull/2693/files#diff-f6abd0868098a19314c3d49490236c56e366983d60c86d919de87985c46998a2R414

The code has moved around since this last comment so I'm not sure what you're referring to, but I made a call to _get_default_timeout several lines below L414.

But it doesn't provide the default timeout, but gets the attribute at all time. If you need the value from the attribute, do call it directly in the code (this is why I don't see the reason for this method), or if you really need the default, do reach out to it directly from the config.

Maybe it's just a naming issue? Though I still think the method is not necessary as it basically just calls one line that you could call directly when needed.

bsipocz

I see test failures, so the tests indeed need to be revisited:

FAILED astroquery/mast/tests/test_mast_remote.py::TestMast::test_zcut_download_cutouts - UnboundLocalError: local variable 'limit_reached' referenced before assignment
FAILED astroquery/mast/tests/test_mast_remote.py::TestMast::test_zcut_get_cutouts - UnboundLocalError: local variable 'limit_reached' referenced before assignment
FAILED astroquery/mast/tests/test_mast_remote.py::TestMast::test_hapcut_download_cutouts - UnboundLocalError: local variable 'limit_reached' referenced before assignment
FAILED astroquery/mast/tests/test_mast_remote.py::TestMast::test_hapcut_get_cutouts - UnboundLocalError: local variable 'limit_reached' referenced before assignment

jaymedina · 2023-04-04T21:13:58Z

Just squashed and rebased, let me know if there's anything else that needs to be addressed before merge. Just a heads up April 7 will be my last day in the office before I leave on vacation for a week @bsipocz @eerovaher

bsipocz

I'm afraid some more iterations for the tests are needed to cover the missing (and not working) input case, as well as to fix checking for the raised warning. But overall the PR is pretty close to being ready.

CHANGES.rst

bsipocz · 2023-04-06T00:47:07Z

astroquery/mast/cutouts.py

+    def _get_default_timeout(self):
+        """ Gets the default request timeout limit. """
+
+        return self._service_api_connection.TIMEOUT


What about L414 then? https://github.com/astropy/astroquery/pull/2693/files#diff-f6abd0868098a19314c3d49490236c56e366983d60c86d919de87985c46998a2R414

astroquery/mast/cutouts.py

astroquery/mast/tests/test_mast_remote.py

astroquery/mast/cutouts.py

docs/mast/mast.rst

astroquery/mast/tests/test_mast_remote.py

bsipocz · 2023-04-06T01:23:16Z

astroquery/mast/tests/test_mast_remote.py

@@ -1016,6 +1021,15 @@ def test_tesscut_get_cutouts_mt(self):
                                     moving_target=True)
            assert error_tica_mt in str(error_msg.value)

+    @pytest.mark.xfail(raises=InputWarning)


This is not the correct approach, xfails are more for cases of known buggy behaviour, or expected server side issues, etc.
Instead, please use pytest.warns context manager, there are plenty of usage examples for it in astroquery.

The reason I had to do it this way is because running with pytest.warns will allow the query to run fully, which will take several minutes and also end up failing the test with the timeout error. This is for the queries that exceed the recommended timeout limit of 600 seconds because the cutout size is too large and the timeout arg hasn't been changed.

jaymedina · 2023-04-06T15:10:14Z

astroquery/mast/cutouts.py

+    if isinstance(size, list) & (mission == 'TESS'):
+        if len(size) == 2:
+            if np.isscalar(size[0]):
+                size = [size[0] * u.pixel, size[1] * u.pixel]


I convert the individual elements in size to pixels rather than the whole data structure because unit conversion for the whole data structure happens 3 lines below, so doing that twice for the pixel unit case would result in a unit of pix^2 which leads to incompatible dimensions being compared. This is the error that comes up:

yes, I would think you don't need the size[0].unit in your example and then it would work (you already have that in the screenshot below)

jaymedina · 2023-04-06T15:13:47Z

astroquery/mast/cutouts.py

+                size = [size[0] * u.pixel, size[1] * u.pixel]
+
+            with u.set_enabled_equivalencies(u.pixel_scale(21 * u.arcsec / u.pixel)):
+                limit_reached = (size * size[0].unit > 30 * u.pixel).any()


For set_enabled_equivalencies, the entire list needs to be assigned a unit rather than the individual elements, in order for a conversion to take place. Otherwise this line results in an error.

here you could put the list into a quantity and then do the check.

size2 = u.Quantity(size2)

So, maybe distinguish in the isinstance conditional between Quantity and not yet quantity inputs?

jaymedina · 2023-04-06T16:41:32Z

astroquery/mast/cutouts.py

+    limit_reached = False
+
+    # Checking 2d size inputs for the recommended cutout size
+    if not isinstance(size, (int, float, u.Quantity)) & (mission == 'TESS'):


My goal here was to catch for any array-like data structures, so lists tuples or np arrays. Unfortunately, I can't call for them explicitly because u.Quantity objects are arrays themselves (see below), so I went with excluding anything that is a single int/float/u.Quantity value, which will implicitly pick up list/tuple/arrays data structures.

Needed parentheses around the not isinstance(size, (int, float, u.Quantity)) - this has been patched, as reflected in the latest build.

jaymedina · 2023-04-06T16:55:07Z

Just made my last commit @bsipocz , this is ready for review. I added comments to certain lines which should hopefully provide more context. Let me know if there's anything else I should address before squashing

bsipocz

two more points to come back to for a tiny bit of a cleanup, but otherwise this will be good to go.

bsipocz · 2023-04-28T22:55:39Z

astroquery/mast/cutouts.py

+                size = [size[0] * u.pixel, size[1] * u.pixel]
+
+            with u.set_enabled_equivalencies(u.pixel_scale(21 * u.arcsec / u.pixel)):
+                limit_reached = (size * size[0].unit > 30 * u.pixel).any()


here you could put the list into a quantity and then do the check.

size2 = u.Quantity(size2)

So, maybe distinguish in the isinstance conditional between Quantity and not yet quantity inputs?

bsipocz · 2023-04-28T23:06:42Z

astroquery/mast/cutouts.py

+    def _get_default_timeout(self):
+        """ Gets the default request timeout limit. """
+
+        return self._service_api_connection.TIMEOUT


But it doesn't provide the default timeout, but gets the attribute at all time. If you need the value from the attribute, do call it directly in the code (this is why I don't see the reason for this method), or if you really need the default, do reach out to it directly from the config.

Maybe it's just a naming issue? Though I still think the method is not necessary as it basically just calls one line that you could call directly when needed.

jaymedina · 2023-05-01T21:50:48Z

@bsipocz I've removed the _get_default_timeout method and am now calling the default timeout directly from Conf().

I'm not sure I understood this comment here, are you suggesting to remove the not isinstance(u.Quantity) conditional in the lines above your comment?

here you could put the list into a quantity and then do the check.

size2 = u.Quantity(size2)

So, maybe distinguish in the isinstance conditional between Quantity and not yet quantity inputs?

bsipocz · 2023-05-02T07:32:59Z

I'm not sure I understood this comment here, are you suggesting to remove the not isinstance(u.Quantity) conditional in > the lines above your comment?

here you could put the list into a quantity and then do the check.
size2 = u.Quantity(size2)
So, maybe distinguish in the isinstance conditional between Quantity and not yet quantity inputs?

I'll do a suggestion commit when next to a computer (travelling atm). Basically I meant to suggest chopping up the conditional to distinguish between vector quantities vs a list of scalar quantities, so the case you show in the screenshot would work.

…rge.Adding arg to change API request timeout cutoff.

… pixel units

jaymedina · 2023-11-28T23:46:07Z

Hi @bsipocz and @keflavich, I'm tagging the interim astroquery.mast maintainer, Julie Imig (@astrojimig). She will be taking over remaining PRs and will be the liaison for MAST-related issues after I'm gone on December 1st.

bsipocz changed the title ~~Cutouts limit~~ MAST: Cutouts limit Mar 29, 2023

bsipocz added the mast label Mar 29, 2023

bsipocz added this to the v0.4.7 milestone Mar 29, 2023

bsipocz reviewed Mar 29, 2023

View reviewed changes

astroquery/mast/cutouts.py Outdated Show resolved Hide resolved

astroquery/mast/cutouts.py Outdated Show resolved Hide resolved

eerovaher reviewed Mar 29, 2023

View reviewed changes

astroquery/mast/cutouts.py Outdated Show resolved Hide resolved

astroquery/mast/cutouts.py Outdated Show resolved Hide resolved

astroquery/mast/cutouts.py Outdated Show resolved Hide resolved

astroquery/mast/cutouts.py Outdated Show resolved Hide resolved

jaymedina marked this pull request as draft March 29, 2023 18:49

jaymedina marked this pull request as ready for review March 30, 2023 17:45

eerovaher reviewed Mar 30, 2023

View reviewed changes

astroquery/mast/cutouts.py Outdated Show resolved Hide resolved

astroquery/mast/cutouts.py Outdated Show resolved Hide resolved

astroquery/mast/cutouts.py Outdated Show resolved Hide resolved

bsipocz reviewed Mar 31, 2023

View reviewed changes

bsipocz requested changes Mar 31, 2023

View reviewed changes

jaymedina force-pushed the cutouts-limit branch 3 times, most recently from 01b70ce to 8de360b Compare April 4, 2023 21:13

jaymedina self-assigned this Apr 4, 2023

bsipocz requested changes Apr 6, 2023

View reviewed changes

jaymedina commented Apr 6, 2023

View reviewed changes

bsipocz reviewed Apr 28, 2023

View reviewed changes

jaymedina force-pushed the cutouts-limit branch from a8b15ad to 9c6ff8b Compare May 5, 2023 16:27

jaymedina mentioned this pull request May 5, 2023

Fixed failing PANSTARRS Catalog queries #2727

Merged

3 tasks

bsipocz force-pushed the cutouts-limit branch from 9c6ff8b to c21f5a0 Compare June 8, 2023 03:09

jaymedina added 2 commits June 7, 2023 20:28

Added Warning message for Tesscut requests when cutout size is too la…

ce2fbf7

…rge.Adding arg to change API request timeout cutoff.

Updated documentation to include commentary on TESSCut request timeouts

c27e3db

jaymedina added 11 commits June 7, 2023 20:28

Simplifying code, updating unit tests, error prevention

a8b6e5d

removing timeout example, skipping doctest for largequerywarning example

324ac45

addressing UnitConversionError for list of scalar inputs converted to…

9c0e6af

… pixel units

array-like cutout sizes added to test_tesscut_timeout_param

0422c8e

patched up resetting the default timeout after timeout is adjusted

33f80b3

asserting that timeout returns to 600s after it is adjusted

7e0ff90

codestyle

8912243

codestyle

3d91932

catching all array-like inputs. lists, tuples, np arrays

c293531

resolved zcut unit test error

093989a

getting default timeout value directly from conf

79e6f05

bsipocz force-pushed the cutouts-limit branch from c21f5a0 to 79e6f05 Compare June 8, 2023 03:28

bsipocz removed this from the v0.4.7 milestone Feb 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAST: Cutouts limit #2693

MAST: Cutouts limit #2693

jaymedina commented Mar 29, 2023 •

edited

codecov bot commented Mar 29, 2023 •

edited

bsipocz left a comment

jaymedina commented Mar 30, 2023

bsipocz Mar 31, 2023

jaymedina Apr 4, 2023

bsipocz Apr 6, 2023

jaymedina Apr 6, 2023

bsipocz Apr 28, 2023

bsipocz left a comment

jaymedina commented Apr 4, 2023 •

edited

bsipocz left a comment

bsipocz Apr 6, 2023

bsipocz Apr 6, 2023

jaymedina Apr 6, 2023

jaymedina Apr 6, 2023 •

edited

bsipocz Apr 28, 2023

jaymedina Apr 6, 2023 •

edited

bsipocz Apr 28, 2023

jaymedina Apr 6, 2023 •

edited

jaymedina Apr 6, 2023 •

edited

jaymedina commented Apr 6, 2023 •

edited

bsipocz left a comment

bsipocz Apr 28, 2023

bsipocz Apr 28, 2023

jaymedina commented May 1, 2023

bsipocz commented May 2, 2023

jaymedina commented Nov 28, 2023 •

edited

MAST: Cutouts limit #2693

Are you sure you want to change the base?

MAST: Cutouts limit #2693

Conversation

jaymedina commented Mar 29, 2023 • edited

NOTE: Screenshots are outdated

codecov bot commented Mar 29, 2023 • edited

Codecov Report

bsipocz left a comment

Choose a reason for hiding this comment

jaymedina commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsipocz left a comment

Choose a reason for hiding this comment

jaymedina commented Apr 4, 2023 • edited

bsipocz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymedina Apr 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymedina Apr 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymedina Apr 6, 2023 • edited

Choose a reason for hiding this comment

jaymedina Apr 6, 2023 • edited

Choose a reason for hiding this comment

jaymedina commented Apr 6, 2023 • edited

bsipocz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymedina commented May 1, 2023

bsipocz commented May 2, 2023

jaymedina commented Nov 28, 2023 • edited

jaymedina commented Mar 29, 2023 •

edited

codecov bot commented Mar 29, 2023 •

edited

jaymedina commented Apr 4, 2023 •

edited

jaymedina Apr 6, 2023 •

edited

jaymedina Apr 6, 2023 •

edited

jaymedina Apr 6, 2023 •

edited

jaymedina Apr 6, 2023 •

edited

jaymedina commented Apr 6, 2023 •

edited

jaymedina commented Nov 28, 2023 •

edited