Add cutouts for casda by jd-au · Pull Request #2366 · astropy/astroquery

jd-au · 2022-04-19T05:13:04Z

Added the ability to produce spatial and spectral cutouts from ASKAP images and cubes using CASDA's SODA implementation.

pep8speaks · 2022-04-19T05:13:07Z

Hello @jd-au! 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-04-22 07:32:51 UTC

bsipocz

I mostly have nitpick comments.

The bigger point is that I feel the module would benefit if the baseclass would be updated to be QueryWithLogin to help with admining the authentication, following the API of the other modules that deal with the need of authentication.

bsipocz · 2022-04-19T21:21:33Z

+                right = fk5_coords.ra.degree + (width/2)
+                pos = 'RANGE {} {} {} {}'.format(left, right, bottom, top)
+            else:
+                pos = 'CIRCLE {} {} {}'.format(fk5_coords.ra.degree, fk5_coords.dec.degree, 1*u.arcmin.to(u.deg))


would it be possible to bring this 1 arcmin default out somewhere to be as a default?

I see that it would change the current API behaviour (there is already a minor change to default from ValueError to this default) to change that height+width takes the first case in the conditional then radius could be defaulted to the 1archmin in the signature?

I've moved this to the signature of the query_region_async , _args_to_payload and cutout functions.

bsipocz · 2022-04-19T21:34:41Z

+            if (band[0] is not None and not isinstance(band[0], u.Quantity)) or (band[1] is not None and not isinstance(band[1], u.Quantity)):
+                raise ValueError("The 'band' value must be a list of 2 wavelength or frequency values.")
+            if band[0] is not None and band[1] is not None and band[0].unit.physical_type != band[1].unit.physical_type:
+                raise ValueError("The 'band' values must have the same kind of units.")
+            if band[0] is not None or band[1] is not None:


nitpick: I feel these could be simplified a bit to not check the same things repeativly? (Though nested conditionals may not be that nice an alternative either).

I tried out a few approaches to simplifying these but none made it much better. I've pull out the band[0] is not None code into variables to hopefully make it more readable though.

bsipocz · 2022-04-19T21:36:41Z

+                request_payload['BAND'] = '{} {}'.format(min_band, max_band)
+
+        if kwargs.get('channel') is not None:
+            channel = kwargs.get('channel')


nitpick: you could bring this out from the conditional, avoiding calling get() twice. Same for band above.

Change made

bsipocz · 2022-04-19T21:38:08Z

+            if not isinstance(channel, (list, tuple)) or len(channel) != 2:
+                raise ValueError("The 'channel' value must be a list of 2 integer values.")
+            if (not isinstance(channel[0], int)) or (not isinstance(channel[1], int)):
+                raise ValueError("The 'channel' value must be a list of 2 integer values.")


nitpick: Maybe have just one long check to avoid duplicates?

Change made

bsipocz · 2022-04-19T21:38:41Z

-            bottom = fk5_coords.dec.degree + (height/2)
-            left = fk5_coords.ra.degree - (width/2)
-            right = fk5_coords.ra.degree + (width/2)
-            pos = 'RANGE {} {} {} {}'.format(left, right, top, bottom)


nitpick: using f-strings maybe easier/nicer to read here and elsewhere?

Change made - that's the first time I have encountered f-strings so thank-you for the suggestion.

yes, we couldn't really use them for a long time, but fortunately, we now dropped the old python versions that don't have it.

bsipocz · 2022-04-19T21:50:16Z

+            log.info("Adding parameters: " + str(cutout_spec))
+        resp = self._request('POST', job_location + '/parameters', data=cutout_spec, cache=False)
+        resp.raise_for_status()
+        return


no need for return here

bsipocz · 2022-04-19T21:52:32Z

+        width : str or `astropy.units.Quantity`, optional
+            the width for a box cutout
+        band : list of `astropy.units.Quantity` with two elements, optional
+            the spectral range to be included, may be low and high wavelengths in metres or low and high frequencies in Hertz. Use None for an open bound.


nitpick: this is a bit long, please break the line at 120 or shorter

I've updated all code in this file to be 120 characters or less

bsipocz · 2022-04-19T21:55:26Z

+The cutout support in AstroQuery allows both spatial and spectral cutouts.
+To produce a spatial cutout, pass in a coordinate and either a radius or a height and a width to the :meth:`~astroquery.casda.CasdaClass.cutout` method.
+To produce a spectral cutout, pass in either a band or a channel value. 
+For band, the value must be a list or tuple of two `astropy.units.Quantity` objects specifying a low and high frequency or wavelength. For an open ended range use `None` as the open value.


nitpick: break the long lines

I've updated the newly added content to be 120 characters or less

bsipocz · 2022-04-19T21:58:38Z

+    >>> import getpass
+    >>> centre = coordinates.SkyCoord.from_name('2MASX J08161181-7039447')
+    >>> username = 'email@somewhere.edu.au'
+    >>> password = getpass.getpass(str("Enter your OPAL password: "))


Overall it would be beneficial to switch to use the authenticated baseclass: QueryWithLogin, it would help you deal with the password promts to follow the usual way we do it in other modules.

bsipocz · 2022-04-19T21:59:31Z

+
+    @pytest.mark.skipif(('CASDA_USER' not in os.environ or
+                        'CASDA_PASSWD' not in os.environ),
+                        reason='Requires real CASDA user/password (CASDA_USER '


Long term it would be nice to figure out some test user that we could use for testing purposes. E.g password stored as a token, or it would have access only some limited test data, etc.

jd-au · 2022-04-20T05:52:47Z

I mostly have nitpick comments.

The bigger point is that I feel the module would benefit if the baseclass would be updated to be QueryWithLogin to help with admining the authentication, following the API of the other modules that deal with the need of authentication.

It might be best to do that as a separate PR, however I will start on it immediately so that both changes are in the same release

bsipocz · 2022-04-20T06:56:18Z

The bigger point is that I feel the module would benefit if the baseclass would be updated to be QueryWithLogin to help with admining the authentication, following the API of the other modules that deal with the need of authentication.

It might be best to do that as a separate PR, however I will start on it immediately so that both changes are in the same release

Yes, I agree that it's beyond the scope here and would better be in a separate PR.

codecov · 2022-04-20T07:08:23Z

Codecov Report

Merging #2366 (aebbb34) into main (1368992) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2366      +/-   ##
==========================================
+ Coverage   63.17%   63.29%   +0.12%     
==========================================
  Files         132      132              
  Lines       17187    17241      +54     
==========================================
+ Hits        10858    10913      +55     
+ Misses       6329     6328       -1

Impacted Files	Coverage Δ
astroquery/casda/core.py	`93.01% <100.00%> (+2.61%)`	⬆️
astroquery/esa/hsa/core.py	`36.55% <0.00%> (+0.88%)`	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

bsipocz

Thank you! I'll commit the one misunderstanding in the previous review and merge once tests pass.

bsipocz · 2022-04-22T04:04:43Z

-            bottom = fk5_coords.dec.degree + (height/2)
-            left = fk5_coords.ra.degree - (width/2)
-            right = fk5_coords.ra.degree + (width/2)
-            pos = 'RANGE {} {} {} {}'.format(left, right, top, bottom)


yes, we couldn't really use them for a long time, but fortunately, we now dropped the old python versions that don't have it.

jd-au · 2022-04-22T04:28:01Z

Thank you! I'll commit the one misunderstanding in the previous review and merge once tests pass.

Thanks for the review! If you have an example of the approach you want with kwarg I can make the change

Also include support for numpy arrays for band and channel, and autocorrect the band and channel values to be in ascending order

bsipocz · 2022-04-22T17:54:21Z

Thanks @jd-au!

jd-au and others added 3 commits December 20, 2021 11:21

Initial 2D cutout implementation

13562b1

Add spectral cutout capability

33f23ee

Change cutout example to use RACS

83ed542

jd-au and others added 3 commits April 19, 2022 15:14

Merge branch 'main' into CASDA-6552-Add-cutouts-for-CASDA

d736003

Formatting fixes

a314e9a

Add a changelog entry

9c4ecf4

jd-au added enhancement casda labels Apr 19, 2022

Doc test update

37dfb93

bsipocz reviewed Apr 19, 2022

View reviewed changes

Tidy up as recommended by review

d09efd7

Formatting fixes

b93e5a7

Improve test coverage

ae0daec

bsipocz approved these changes Apr 22, 2022

View reviewed changes

Make kwargs kwarg only

c69651f

bsipocz force-pushed the CASDA-6552-Add-cutouts-for-CASDA branch from 944f149 to c69651f Compare April 22, 2022 04:22

jd-au added 2 commits April 22, 2022 17:23

Add a spectral cutout example

d635433

Also include support for numpy arrays for band and channel, and autocorrect the band and channel values to be in ascending order

Tidy whitespace

aebbb34

bsipocz merged commit 65846ed into astropy:main Apr 22, 2022

bsipocz added this to the v0.4.7 milestone Apr 22, 2022

jd-au deleted the CASDA-6552-Add-cutouts-for-CASDA branch April 22, 2022 22:22

Uh oh!

Uh oh!

Conversation

jd-au commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2022-04-22 07:32:51 UTC

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jd-au commented Apr 20, 2022

Uh oh!

bsipocz commented Apr 20, 2022

Uh oh!

codecov Bot commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jd-au commented Apr 22, 2022

Uh oh!

bsipocz commented Apr 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jd-au commented Apr 19, 2022 •

edited

Loading

pep8speaks commented Apr 19, 2022 •

edited

Loading

codecov Bot commented Apr 20, 2022 •

edited

Loading