MAST: Moving target TESScut #2121

ceb8 · 2021-07-19T18:22:37Z

Adding functionality to access the new MAST Moving Target TESScut functionality from the astroquery.mast Tesscut class.

Includes:

New moving_target argument to Tesscut class functions, that allows users to make cutouts for named moving targets (asteroids/comets etc).
Associated documentation
Associated tests (remote and local)
Update to mast cloud access documentation to reflect prior code changes (unrelated, just apparently forgot to do the docs when I updated the code)
General test tweaks to explicitly catch warnings and make sure all the remote tests pass

pep8speaks · 2021-07-19T18:22:40Z

Hello @ceb8! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-01-31 20:24:21 UTC

codecov · 2021-07-19T18:48:08Z

Codecov Report

Merging #2121 (2a127ae) into main (31a4164) will increase coverage by 0.02%.
The diff coverage is 86.04%.

@@            Coverage Diff             @@
##             main    #2121      +/-   ##
==========================================
+ Coverage   62.70%   62.72%   +0.02%     
==========================================
  Files         131      131              
  Lines       16828    16855      +27     
==========================================
+ Hits        10552    10573      +21     
- Misses       6276     6282       +6

Impacted Files	Coverage Δ
astroquery/mast/cutouts.py	`79.42% <86.04%> (-0.25%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31a4164...2a127ae. Read the comment docs.

bsipocz

This certainly needs a changelog entry.

Most of my comments are nit picks, but while I see why it's easier for the backend to use a new kwargs, for UX I don't think we need a new kwarg to specify the name.

Some of the test/cloud changes are unrelated to this PR, now it's fine to leave them, but next time I think they worth to put them in separate, smaller PRs.

I also have one remote failure:

astroquery/mast/tests/test_mast.py .........................................                                                                  [ 51%]
astroquery/mast/tests/test_mast_remote.py .........................F...........                                                               [ 98%]
docs/mast/mast.rst s                                                                                                                          [100%]

===================================================================== FAILURES ======================================================================
_______________________________________________________ TestMast.test_catalogs_query_criteria _______________________________________________________

self = <astroquery.mast.tests.test_mast_remote.TestMast object at 0x14daf9d00>

    def test_catalogs_query_criteria(self):
    
        # clear columns config
        mast.Catalogs._column_configs = dict()
    
        # without position
        result = mast.Catalogs.query_criteria(catalog="Tic", Bmag=[30, 50], objType="STAR")
        assert isinstance(result, Table)
        assert len(result) >= 10
        assert result[np.where(result['ID'] == '81609218')]
    
        result = mast.Catalogs.query_criteria(catalog="ctl", Tmag=[10.5, 11], POSflag="2mass")
        assert isinstance(result, Table)
>       assert len(result) >= 400
E       assert 316 >= 400
E        +  where 316 = len(<Table masked=True length=316>\n    ID    version   HIP      TYC      ... e_Dec_orig raddflag wdflag   objID  \n   str9 ......       70.0        1      0 137129648\n394245807 20190415    -- 9490-00757-1 ...      130.0        1      0 516829248)

astroquery/mast/tests/test_mast_remote.py:463: AssertionError

bsipocz · 2021-08-10T21:55:30Z

astroquery/mast/cutouts.py

        self._service_api_connection.set_service_params(services, "tesscut")

-    def get_sectors(self, coordinates=None, radius=0*u.deg, objectname=None):
+    def get_sectors(self, coordinates=None, radius=0*u.deg, objectname=None, moving_target=None, mt_type=None):


maybe now is a good time to make the kwargs mandatory kwargs given that only one of them can be provided?

Suggested change

def get_sectors(self, coordinates=None, radius=0*u.deg, objectname=None, moving_target=None, mt_type=None):

def get_sectors(self, *, coordinates=None, radius=0*u.deg, objectname=None, moving_target=None, mt_type=None):

Whatever gets included in * also needs to be added to params = {"obj_id": moving_target} in L157 before the service call right?

* means that the rest of the parameters are keyword only, nothing additional is included in it.

Oh I see - learned something new today. This might be something worth starting a new PR over, since its a syntax change, we'd want to add the * to all the Zcut methods as well, etc. What do you think?

yes, please do add it to all places where it makes sense, as separate PR sounds perfect, as it would allow us flexibility for the future as well as a safety-net against user bugs.

bsipocz · 2021-08-10T21:57:27Z

astroquery/mast/cutouts.py

@@ -164,7 +195,8 @@ def get_sectors(self, coordinates=None, radius=0*u.deg, objectname=None):
            warnings.warn("Coordinates are not in any TESS sector.", NoResultsWarning)
        return Table(sector_dict)

-    def download_cutouts(self, coordinates=None, size=5, sector=None, path=".", inflate=True, objectname=None):
+    def download_cutouts(self, coordinates=None, size=5, sector=None, path=".", inflate=True,


same here, make them kwargs only

I've made a separate issue for this: #2267

bsipocz · 2021-08-13T21:30:38Z

astroquery/mast/cutouts.py

+        else:
+
+            # Get Skycoord object for coordinates/object
+            coordinates = parse_input_location(coordinates, objectname)


I see that this has been the same before, but maybe it's worth using a different name here and below in the other methods, too that coordinates that is also a non-parsed, and maybe None input kwarg.

We rec would be to file and issue and have this be a seperate PR, bc this is the convention throughout the MAST module, so I think it would be clearer if they all got changed at once.

bsipocz · 2021-08-13T21:37:16Z

astroquery/mast/cutouts.py

        objectname : str, optional
            The target around which to search, by name (objectname="M104")
            or TIC ID (objectname="TIC 141914082").
-            One and only one of coordinates and objectname must be supplied.
+            One and only one of coordinates, objectname, moving_target must be supplied.
+        moving_target : str, optional


Why do we need a new kwarg for moving targets? At the end of the day they are the name of the object to query, and from the user's pow I don't see a difference whether querying for M1 or Ceres.

Hi @bsipocz , I've been tasked with addressing threads for this PR so it can get merged: From what I can tell it looks like there are two separate databases, and setting moving_target will make the request call for the moving target database in MAST.

Hi, so my reason for doing this was twofold:

The resolving method for moving vs regular targets is completely different, and naming in astronomy is not always unique so I was worried about the user putting in a valid moving target and getting a stationary result.

The output for moving targets is not quite the same as for regular targets, since it's a different type of cutout, so I thought it was clearer to keep the two queries more separate.

Both of these concerns could be addressed by having a moving target flag that defaults to false instead of a whole new argument.

The naming mess affects not just moving targets, I'm sure there is overlap within non-moving ones if we look closely, too, and also, the users don't need to know what's under the hood, that depending on the type of the object the query goes to a different database.
So, I find the logic of having a bool moving_target clearer than basically having two separate APIs for the name depending on the service.

I've made a new commit that turns moving_target into an optional bool argument. With your approvals, I'll make another commit to update the docstrings, tests, documentation, and collapse the commits into one.

bsipocz · 2021-08-13T21:45:25Z

astroquery/mast/observations.py

-                    self._cloud_connection.download_file(data_product, local_path, cache)
-                except Exception as ex:
-                    log.exception("Error pulling from S3 bucket: {}".format(ex))
+            if self._cloud_connection is not None:


how is this related to this PR?

This looks like an unrelated commit that was part of something else. I can make another commit to reverse these changes on my local branch.

Probably this was something else I was working on that accidentally got in. @jaymedina you might want to check what is on the main branch, and if this bit needs to be added in another PR or not.

I'm going to revert these changes to what's in the main branch because it's following the same logic just written slightly differently, without the fall_back trigger and a few other things.

bsipocz · 2021-08-13T21:46:26Z

docs/mast/mast.rst

+
+.. code-block:: python
+
+                >>> from astroquery.mast import Observations


this looks overindented

Just checked the rest of mast.rst and the indentation is consistent with the rest of the document. I worry pulling it back will mess up the formatting.

OK, then this is an issue for a separate quick cleanup PR.

bsipocz · 2021-08-13T21:52:59Z

docs/mast/mast.rst

+                >>> single_obs = Observations.query_criteria(obs_collection="IUE",obs_id="lwp13058")
+                >>> data_products = Observations.get_product_list(single_obs)
+
+                >>> Observations.download_products(data_products,


download_products returns a table, maybe it's worth assigning it into a variable. However, it stays as is (I really don't have a strong preference), then include the table in the output below.

bsipocz · 2021-08-13T21:55:58Z

docs/mast/mast.rst

+
+.. code-block:: python
+
+                >>> from astroquery.mast import Tesscut


I run into a timeout with this example

@jaymedina Try this example now, if I remember correctly the JPL horizons service was having timeout problems last August, so that might have been the problem.

Looks good

In [3]: hdulist = Tesscut.get_cutouts(moving_target="Eleonora", size=5, sector=6 ...: ) In [4]: hdulist[0].info() Filename: <class '_io.BytesIO'> No. Name Ver Type Cards Dimensions Format 0 PRIMARY 1 PrimaryHDU 54 () 1 PIXELS 1 BinTableHDU 150 355R x 16C [D, E, J, 25J, 25E, 25E, 25E, 25E, J, E, E, 38A, D, D, D, D] 2 APERTURE 1 ImageHDU 97 (2136, 2078) int32

Looks like we have 355 rows now instead of 356. Must be a numpy thing?

@ceb8 would you mind giving me writing access to your forked astroquery repo? I have some updates I need to commit to this branch.

@jaymedina you should have an invite now

jaymedina · 2022-01-20T19:53:01Z

I'll have to dig myself out of this again. In the meantime, tests look good. There's one test that was intentionally skipped because it retrieves too much data, which I'll revisit later - it's lower priority because I have some back-end tests running and this test executes, I'll take a look at it in the next sprint and see if I can recreate it for the GitHub tests:

test_mast_remote.py ..................s....................                                                                                                                                    [100%]

====================================================================================== short test summary info =======================================================================================
SKIPPED [1] test_mast_remote.py:260: Tests should not download this much data. Skipping until revised.
============================================================================= 38 passed, 1 skipped in 1521.35s (0:25:21) =============================================================================

jaymedina · 2022-01-20T20:06:45Z

I'm assuming a rebase will collapse all the commits that got merged from recent changes to main @bsipocz @ceb8

ceb8 · 2022-01-20T20:12:21Z

@jaymedina Yes, a rebase is definitely in order, if you do that and are still having issued with stray commits, let me know. We might have to back out your changes, then rebase, then add them back in.

jaymedina · 2022-01-28T17:26:25Z

Heads up @ceb8 & @bsipocz : I'm going to move forward with those changes I made in commit 6e91621 unless there are any objections.

jaymedina · 2022-01-28T20:55:51Z

Since moving_target will now be a bool value, moving_target and objectname will now be required to go together as specified in the dosctrings. Tests will account for the possible user input combinations below:

moving_target only
coordinates only (correct)
objectname only
moving_target + coordinates
moving_target + objectname (correct)
coordinates + objectname
moving_target + coordinates + objectname

Update: Written tests results

test_mast.py .........................................                                                              [100%]

=================================================== 41 passed in 1.08s ====================================================

test_mast_remote.py ..................s....................                                                         [100%]

======================================== 38 passed, 1 skipped in 377.37s (0:06:17) ========================================

jaymedina · 2022-01-29T00:35:13Z

The old dependencies and readthedocs checks are failing from not having the recent merge. Is it alright if I rebase really quick @bsipocz ? I have a backup branch ready.

bsipocz · 2022-01-29T01:24:44Z

The old dependencies and readthedocs checks are failing from not having the recent merge. Is it alright if I rebase really quick @bsipocz ? I have a backup branch ready.

No need to ask, do feel free to rebase, squash commits, etc as often as you need :)

…fixes.

…, explicitly calling table variable to download_products output, and reverting changes made to observations.py

…r ID input

…wnload_cutouts, and get_cutouts.

…s, download_cutouts, and get_cutouts. also fixing import statements and other typos.

ceb8 · 2022-01-30T17:01:07Z

@jaymedina This is looking good. Is it ready to be merged, or are you planning to do anything else?

jaymedina · 2022-01-30T20:36:11Z

@jaymedina This is looking good. Is it ready to be merged, or are you planning to do anything else?

Nope I’m done with the PR. Now just trying to figure out why the readthedocs check is failing, but the error messages aren’t giving me any hints.

ceb8 · 2022-01-30T20:47:36Z

@jaymedina OK, I'll give it a last look over, and see whats up with the docs check in the next day or so.

docs/mast/mast.rst

ceb8 · 2022-01-31T20:41:09Z

Thanks @jaymedina !

jaymedina · 2022-01-31T20:41:48Z

Thank you :) @ceb8

bsipocz · 2022-01-31T20:51:18Z

This needs a changelog entry, please open a separate PR for adding one, with using the number of this PR.

Changelog update for PR #2121

ceb8 added the mast label Jul 19, 2021

bsipocz added this to the v0.4.4 milestone Aug 10, 2021

bsipocz reviewed Aug 13, 2021

View reviewed changes

bsipocz modified the milestones: v0.4.4, v4 Nov 13, 2021

ceb8 force-pushed the moving_target_tesscut branch 2 times, most recently from 6b04633 to 0b9c924 Compare January 20, 2022 22:00

jaymedina force-pushed the moving_target_tesscut branch from be272f0 to 6e91621 Compare January 26, 2022 16:04

jaymedina force-pushed the moving_target_tesscut branch 3 times, most recently from 9110923 to 32c5844 Compare January 28, 2022 20:43

jaymedina force-pushed the moving_target_tesscut branch 2 times, most recently from ca87911 to ee0766d Compare January 29, 2022 01:05

Clara Brasseur and others added 7 commits January 28, 2022 20:27

adding moving target tesscut and docs

fd2a9cc

adding moving tesscut tests

8b0dbd5

adding a few more tests for error conditions. also making code style …

1dbe04f

…fixes.

second pass thru the PR: fixed typo of 355Rows corrected from 356Rows…

6aa80d0

…, explicitly calling table variable to download_products output, and reverting changes made to observations.py

setting moving_target option to bool instead of its own target name o…

c9ebd67

…r ID input

updating moving_target argument and documentation for get_sectors, do…

4aec2b3

…wnload_cutouts, and get_cutouts.

updating moving target functionality and test coverage for get_sector…

336e657

…s, download_cutouts, and get_cutouts. also fixing import statements and other typos.

jaymedina force-pushed the moving_target_tesscut branch from ee0766d to 336e657 Compare January 29, 2022 01:27

jaymedina added 4 commits January 28, 2022 20:31

pep8

3a33c66

updating documentation for moving_target functionality as bool

1a45e6f

removing an indent added by mistake

3f33ecc

readthedocs indentation

ba37d2a

eerovaher reviewed Jan 31, 2022

View reviewed changes

docs/mast/mast.rst Outdated Show resolved Hide resolved

small docs typo

2a127ae

ceb8 merged commit b3b54e9 into astropy:main Jan 31, 2022

jaymedina deleted the moving_target_tesscut branch January 31, 2022 20:42

bsipocz modified the milestones: v4, v0.4.6 Jan 31, 2022

ceb8 mentioned this pull request Jan 31, 2022

Changelog update for PR #2121 #2270

Merged

bsipocz added a commit that referenced this pull request Jan 31, 2022

Merge pull request #2270 from ceb8/fix_moving_tesscut_changelog

cda88ef

Changelog update for PR #2121

jaymedina mentioned this pull request Jan 31, 2022

changelog entry for PR 2121 #2271

Closed

	def get_sectors(self, coordinates=None, radius=0*u.deg, objectname=None, moving_target=None, mt_type=None):
	def get_sectors(self, , coordinates=None, radius=0u.deg, objectname=None, moving_target=None, mt_type=None):


		.. code-block:: python

		>>> from astroquery.mast import Observations


		.. code-block:: python

		>>> from astroquery.mast import Tesscut

MAST: Moving target TESScut #2121

MAST: Moving target TESScut #2121

Conversation

ceb8 commented Jul 19, 2021

pep8speaks commented Jul 19, 2021 • edited

Comment last updated at 2022-01-31 20:24:21 UTC

codecov bot commented Jul 19, 2021 • edited

Codecov Report

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

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

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

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

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

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

jaymedina commented Jan 20, 2022

jaymedina commented Jan 20, 2022

ceb8 commented Jan 20, 2022

jaymedina commented Jan 28, 2022

jaymedina commented Jan 28, 2022 • edited

jaymedina commented Jan 29, 2022

bsipocz commented Jan 29, 2022

ceb8 commented Jan 30, 2022

jaymedina commented Jan 30, 2022

ceb8 commented Jan 30, 2022

ceb8 commented Jan 31, 2022

jaymedina commented Jan 31, 2022

bsipocz commented Jan 31, 2022

pep8speaks commented Jul 19, 2021 •

edited

codecov bot commented Jul 19, 2021 •

edited

jaymedina commented Jan 28, 2022 •

edited