Desi legacy survey

[burnout87]

No description provided.

[pep8speaks]

Hello @burnout87! 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-06-15 13:01:46 UTC

[burnout87]

As advised in #2192 , the various renamings have been applied

[codecov]

Codecov Report

Merging #2210 (7cc8d8b) into main (fc3b18a) will increase coverage by 0.05%.
The diff coverage is 91.42%.

@@            Coverage Diff             @@
##             main    #2210      +/-   ##
==========================================
+ Coverage   63.95%   64.01%   +0.05%     
==========================================
  Files         132      133       +1     
  Lines       16974    17009      +35     
==========================================
+ Hits        10856    10888      +32     
- Misses       6118     6121       +3     

Impacted Files	Coverage Δ
astroquery/utils/commons.py	77.24% <ø> (ø)
astroquery/desi/core.py	91.42% <91.42%> (ø)

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

[keflavich]

Thanks. We still need a few things before this is merged:

1. Documentation. At least basic documentation describing the purpose and use of this tool should be added as docs/desi.rst
2. Non-remote tests. At least one non-remote test is needed - we can help guide you in how to do this, but following existing examples from other modules is probably the best way to go.
3. Cleanup. There is a bunch of commented-out text from the tutorial. This text is harmless, but we'd prefer to have it removed before merging.

[bsipocz]

@burnout87 - thanks for the PR. Before going into a more formal/thorough review, could you please rebase to get rid of the merge commits, as well as to squash the commits that add the test files? Those files need to be cut back a lot in size, currently, the new module's test data takes up 75+% of all of astroquery's size. Thanks!

[ceb8]

This is looking very good! Most of my comments are small, the two bigger items are:

1. The examples in the docs don't run as-is.
2. The async functions don't seem to actually be async, in that they don't return HTTP responses. They either need to follow the async API specs or not be called async.

[bsipocz]

My comment above still stands, please rebase/squash out the commits so bigger test files are not added and then removed in the history

[burnout87]

Thanks for your time. The first iteration of comments has been addressed.

[burnout87]

My comment above still stands, please rebase/squash out the commits so bigger test files are not added and then removed in the history

Once the last conversation will be resolved I will proceed to do that.

[burnout87]

My comment above still stands, please rebase/squash out the commits so bigger test files are not added and then removed in the history

Commits squashing executed.

[bsipocz]

It's getting there, thanks.

Here is my first round of review, most of the items are minor, and are needed to fix the failures we see in CI, etc.

[bsipocz]

add all the parameters to the docs, including the optional ones, too

[bsipocz]

you need to write the whole example script as one big block, along with the output, have a look at the other modules (e.g. cadc or ipac.irsa) for examples. If you run the remote tests on this file, edit it until it passes. (python setup.py test -t docs/desi/desi.rst -R)

[keflavich]

I believe this issue is still holding tests back from passing

[bsipocz]

@burnout87 - I'll come back to this later during the week for another round of review and minor fixes that would fix e.g. the docs build.

[burnout87]

@burnout87 - I'll come back to this later during the week for another round of review and minor fixes that would fix e.g. the docs build.

Hi @bsipocz , I was wondering if you had time for the last round of review. We didnt interact during the last weeks, so I lost track of this. Thanks

[bsipocz]

Some major and minor nitpicking comments. I'm pushing the fix for the docs build separately as it's an automodapi peculiarity.

Also, as you've squashed down all the commits to one you lost the authorship credit. I'm happy to get some of that fixed by sorting out the duplicated commits/extra data etc squashes from the backup branch once we're otherwise ready to merge.

[bsipocz]

rephrase that this is a box search rather than a radius

[keflavich]

this is still incorrectly specified in the docstring

[bsipocz]

are you sure you don't want to raise this as an exception? Why and when does this return an HTTPError?

[burnout87]

An HTTPError happens in the case of not valid inputs, more specifically when these are outside of the survey coverage.

We, for our internal applications, want to display a warning message in case this situation occurs.

[bsipocz]

yes, my point is that if the URL throws an error then I think we should throw it, too. E.g. the user expects a file as a return, but here they get a valid return yet no files.
Is there any use case where this is more useful for the users than to receive an error?

[burnout87]

The server returns an HTTPError in the case of internal error of the legacysurvey but also in the case I mentioned (ie case of not valid inputs, outside of the survey coverage), so at the moment is impossible for us to distinguish these two situations.

For this reason, we decided just to show the message in the warning to the user in both of those cases.

[burnout87]

Hi @bsipocz , what do you think? Does it make sense to you?

[bsipocz]

It's OK to customize the message but my point still stands, it should the raised. I don't see a use case where a warning rather than an error is preferred after all the query errored out and the None return is not really a valid results for these cases (None return would be fine if there is a valid query and there is no match for ti in the DB, etc.)

[burnout87]

Also, as you've squashed down all the commits to one you lost the authorship credit. I'm happy to get some of that fixed by sorting out the duplicated commits/extra data etc squashes from the backup branch once we're otherwise ready to merge.

Thanks! I was not considering that when doing the squash.

[keflavich]

Following @bsipocz 's unresolved comment: if the download was unsuccessful, we should raise an Exception.

Suggested change

	warnings.warn(f"{str(exp)} - Problem retrieving the file at the url: {image_url}", NoResultsWarning)
	raise urllib.error.HTTPError(f"{str(exp)} - Problem retrieving the file at the url: {image_url}")

[keflavich]

I'm a bit confused by our current error:
https://github.com/astropy/astroquery/actions/runs/3345568625/jobs/5541264991#step:5:673
It's an "unclosed file" ResourceWarning: unclosed file <_io.BufferedReader name='/home/runner/work/astroquery/astroquery/.tox/py310-test-alldeps-devdeps-cov/lib/python3.10/site-packages/astroquery/desi/tests/data/hdu_list_images.fits'>, but shouldn't the file get closed when it leaves scope? I don't recall having to explicitly close .fits files (or load them with a context manager) in other packages.

[keflavich]

There was a suggestion earlier to make radius an optional keyword argument, like this:

Suggested change

	def query_region(self, coordinates, radius=0.5*u.arcmin, *, data_release=9):
	def query_region(self, coordinates, *, radius=0.5*u.arcmin, data_release=9):

I resolved the conversation unintentionally, but I'll re-raise it now: is there any reason not to have it be optional?

[eerovaher]

In some modules the region can be a circle or a rectangle, and in those cases radius should definitely be a keyword-only argument. Although here the region can only be a circle, it might still better to make it a keyword-only argument for consistency's sake with the other sub-packages, and to make it very explicit to all users that the region is indeed a circle and not a rectangle.

[keflavich]

...actually it is a (projected) square, so radius is the wrong kwarg.

Suggested change

	def query_region(self, coordinates, radius=0.5*u.arcmin, *, data_release=9):
	def query_region(self, coordinates, width=0.5*u.arcmin, *, data_release=9):

https://github.com/astropy/astroquery/pull/2210/files#diff-79317ce77daf56401a2a4150461a92c123e7061152b9219d313ac47515f9ad2aR44-R48

Probably this should be refactored to have width and height, and maybe even specifically ra_width and dec_height, as arguments.

[bsipocz]

This comment still needs to be addressed. Also, keep the width mandatory, too by moving the ,* , one place forward as it was suggested in the earlier comment.

[bsipocz]

There are some minor nitpicks. The biggest issue is that the remote test takes a really long time.

OTOH, I went ahead and rebased, so will go ahead, push that rebase to see how the CI react as well as I'll push one minor commit that addresses my comments.

[bsipocz]

this doctest fails from here on

[bsipocz]

Remote tests take a really long time, they need to be significantly cut back (probably a smaller query would do it? I haven't tried)

[bsipocz]

do parametrize on the variables rather than on a boolean. There are plenty of examples in the other modules

[bsipocz]

What about testing for other data releases?

[bsipocz]

This comment still needs to be addressed. Also, keep the width mandatory, too by moving the ,* , one place forward as it was suggested in the earlier comment.

[bsipocz]

The case when only the mandatory arguments are used are not tested, and actually raise exceptions. The code should be fixed to handle the case to fall back to the default width and pixels.

Also, pixels are not documented, please add a one liner to the docstring.

[bsipocz]

@burnout87 - This is rather close to be able to be merged, but currently there are a few cases where the code run into regression. Also, it would be nice to actually test more of the parameter space of the possible input values, e.g. the used data release is very much hardwired. I expect all this remaining stuff can be fixed with a reasonably straightforward commit, but do let us know if there are any questions.

[emoravec]

Thank you very much for making DESI LS a part of astroquery. Over the past few weeks I have been using the branch that contains DESILegacySurvey.query_region() and I've noticed a bug. I have run the following command for about 20 different coordinate pairs that is along the lines of DESILegacySurvey.query_region(coords, width=Angle(5, unit='arcmin'), data_release=9). The query table that it returns has the correct RA range (+- 5') and the Dec range is + ~5 armin but minus typically ~60 degrees. Thus I am getting a lot of extra data and sometimes these queries take a really long time to get the data. See the image below where the "box" (it doesn't look like a box due to the wide range of dec returned in the query) outlined in dotted lines is the region in which I would expect the query to return data ([RA-5',Dec-5'],[RA-5',Dec+5'],[RA+5',Dec+5'],[RA+5',Dec-5']) and the blue dots show the full range of data returned by the query.

I believe I’ve found the problem in the sql query in that there is no bound in the lower limit of Dec.

Currently the sql query is:
qstr = (f"SELECT all * FROM ls_dr{data_release}.tractor WHERE " f"dec<{(coordinates_transformed.dec + width).to_value(u.deg)} and " f"ra>{coordinates_transformed.ra.to_value(u.deg) - width.to_value(u.deg) / np.cos(coordinates_transformed.dec)} and " f"ra<{coordinates_transformed.ra.to_value(u.deg) + width.to_value(u.deg) / np.cos(coordinates_transformed.dec)}")

But I think it should be:
qstr = (f"SELECT all * FROM ls_dr{data_release}.tractor WHERE " f"dec<{(coordinates_transformed.dec + width).to_value(u.deg)} and " f"dec>{(coordinates_transformed.dec - width).to_value(u.deg)} and " f"ra>{coordinates_transformed.ra.to_value(u.deg) - width.to_value(u.deg) / np.cos(coordinates_transformed.dec)} and " f"ra<{coordinates_transformed.ra.to_value(u.deg) + width.to_value(u.deg) / np.cos(coordinates_transformed.dec)}")

[emoravec]

DR10 for DESI LS just came out in December 2022. It would be great to be able to access the DR10 data via a similar command DESILegacySurvey.query_region(coords, width=Angle(30, unit='arcsec'), data_release=10).