Access to NOIRLab Astro Data Archive #3359

weaverba137 · 2025-06-26T22:12:36Z

This PR closes #1701.

The initial commit simply restores the NOIRLab files to the state and content they were when #1701 was last worked on. Further work and testing will be needed to meet astroquery standards.

weaverba137 · 2025-06-26T22:42:39Z

TO DO items identified (i.e. before even starting the review process):

Proofread noirlab/noirlab.rst and verify examples.
Remove telescope holdings from init file. These are certainly outdated. Link to the about page.
Check required acknowledgment(s).
Get remote tests working.
Add non-remote tests to increase coverage, e.g. monkeypatch.

codecov · 2025-06-26T23:06:48Z

Codecov Report

❌ Patch coverage is 96.84211% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.63%. Comparing base (82cab62) to head (f643b79).
⚠️ Report is 83 commits behind head on main.

Files with missing lines	Patch %	Lines
astroquery/noirlab/core.py	96.59%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3359      +/-   ##
==========================================
+ Coverage   70.07%   70.63%   +0.56%     
==========================================
  Files         232      234       +2     
  Lines       19893    20092     +199     
==========================================
+ Hits        13940    14192     +252     
+ Misses       5953     5900      -53

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

weaverba137 · 2025-06-26T23:19:27Z

Note to self: tox -e py310-test-online -- -P noirlab to do remote tests on only astroquery.noirlab.

bsipocz · 2025-06-27T18:38:37Z

You also need -R to run the remote tests

weaverba137 · 2025-07-17T23:56:14Z

@keflavich @bsipocz this is ready for first-pass review.

I'm having some trouble with the doc build:

reading sources... [100%] xmatch/xmatch

/home/docs/checkouts/readthedocs.org/user_builds/astroquery/checkouts/3359/docs/noirlab/noirlab.rst:171: CRITICAL: Title level inconsistent:

astroquery.noirlab Package
-------------------------- [docutils]
/home/docs/checkouts/readthedocs.org/user_builds/astroquery/checkouts/3359/docs/noirlab/noirlab.rst:176: CRITICAL: Title level inconsistent:

Classes
^^^^^^^ [docutils]
looking for now-outdated files... none found

I've tried to set up the title levels similar to other packages, such as sdss.

I'm also looking for advice on whether further application of async_to_sync is needed.

weaverba137 · 2025-07-17T23:58:06Z

PS, the .retrieve() method needs further testing. In the previous PR, this method actually returned a HDUList rather than a filename or file object. Could you please advise on how this should be handled in line with similar astroquery methods?

bsipocz · 2025-07-18T00:07:51Z

Thanks. I need to get back to the ESO review first, but put this on the list now, too.

bsipocz

We did a partial review with some comments, I'm coming back to finish this later this week so no need to address any of these yet.

astroquery/noirlab/core.py

bsipocz · 2025-08-12T19:42:49Z

astroquery/noirlab/core.py

+        rows = [[row[n] for n in names] for row in response_json[1:]]
+        return astropy.table.Table(names=names, rows=rows)
+
+    def service_metadata(self, hdu=False, cache=True):


Is this an end user method or should rather be something private?

I'm not really sure how or whether this would be used by end users. Let me think about that a bit more.

astroquery/noirlab/core.py

bsipocz · 2025-08-12T19:50:50Z

astroquery/noirlab/core.py

+    def query_region_async(self, coordinate, radius=0.1, hdu=False, cache=True):
+        """Query for NOIRLab observations by region of the sky.
+
+        Given a sky coordinate and radius, returns a `~astropy.table.Table`
+        of NOIRLab observations.
+
+        Parameters
+        ----------
+        coordinate : :class:`str` or `~astropy.coordinates` object
+            The target region which to search. It may be specified as a
+            string or as the appropriate `~astropy.coordinates` object.
+        radius : :class:`str` or `~astropy.units.Quantity` object, optional
+            Default 0.1 degrees.
+            The string must be parsable by `~astropy.coordinates.Angle`. The
+            appropriate `~astropy.units.Quantity` object from
+            `~astropy.units` may also be used.
+        hdu : :class:`bool`, optional
+            If ``True`` return the URL for HDU-based queries.
+        cache : :class:`bool`, optional
+            If ``True`` cache the result locally.
+
+        Returns
+        -------
+        :class:`~requests.Response`
+            Response object.
+        """
+        self._validate_version()
+        ra, dec = coordinate.to_string('decimal').split()
+        url = f'{self._sia_url(hdu=hdu)}?POS={ra},{dec}&SIZE={radius}&VERB=3&format=json'
+        response = self._request('GET', url, timeout=self.TIMEOUT, cache=cache)
+        # response.raise_for_status()
+        return response


We may want to have async as a kwarg instead and not have two separate methods. See how we do this in some of the other modules e.g. in cadc or ipac.irsa (or the esa modules).

As far as I can tell cadc still has the old @async_to_sync setup. I took a look at ipac.irsa, and there it is apparent that the meaning of async_job is to submit asynchronous queries to a job system of some kind. That doesn't apply to the NOIRLab API.

Based on my experience with sdss, historically "async" meant "return the response object from self._request(), while "sync" meant "based on the response object, download the files referenced in it". This is definitely more applicable to NOIRLab.

Can you elaborate further on the different meanings of "async"?

bsipocz · 2025-08-12T19:59:21Z

astroquery/noirlab/core.py

+        # response.raise_for_status()
+        return response
+
+    def core_fields(self, hdu=False, cache=True):


How would you feel about having just one method for these three with a kwarg for 'core', 'aux', and 'categoricals'? e.g. list_fields(...)

Good suggestion, I will work on that.

weaverba137 · 2025-08-13T00:03:21Z

@bsipocz, thank you, the suggestions all look good so far, but given my schedule it will be easier to address them all when you're done.

bsipocz · 2025-08-13T00:11:48Z

Yes, totally understandable. I really just submitted this half baked review as that is how far we got during the review tutorial during the summer school I was teaching at. I'll get back to the rest later and will write up an actually usable summary, as there are a couple of things that will need to be fixed while the rest is really just some potential follow-up topics.

And again, thanks for working on this, getting a working noirlab module will be superb!

bsipocz

I have some more comments to finish up my review.

The overarching theme is to make the API a little bit more consistent with the other modules. And maybe consider using pyvo, but that one is optional for this PR.

And in the meantime it looks like the upstream API has changed, so that may mean some tweaks will be necessary (though the tests are primarily failing with the API number comparison).

Also, the docs build runs into some sphinx errors due to API heading inconsistencies, I'm happy to fix up those before merging.

astroquery/noirlab/core.py

bsipocz · 2025-09-13T00:21:24Z

astroquery/noirlab/core.py

+                   f'{self.api_version} from the API.')
+            raise RemoteServiceError(msg)
+
+    def _sia_url(self, hdu=False):


We're not super consistent yet, but the other modules have either a simple sia_url attribute or a property. In the other modules there is a sia_url property instead of a private method

I can promote that to a public method, but there are genuinely two different SIA URLs in the NOIRLab API, one for queries on whole files, the other for HDUs within files. For example, DECam files contain many HDUs, each of which can be considered a separate image.

bsipocz · 2025-09-13T00:23:10Z

astroquery/noirlab/core.py

+        self._validate_version()
+        ra, dec = coordinate.to_string('decimal').split()
+        url = f'{self._sia_url(hdu=hdu)}?POS={ra},{dec}&SIZE={radius}&VERB=3&format=json'
+        response = self._request('GET', url, timeout=self.TIMEOUT, cache=cache)
+        # response.raise_for_status()
+        return response


I wonder if you would consider using pyvo here rather than manually writing the SIA query? It would make things a bit more consistent and easier to maintain even without noirlab interaction
(even if the answer is yes, that can come in a follow-up)

This is definitely a good idea, but I would need to get more familiar with pyvo, so I am in favor postponing that to a separate PR.

astroquery/noirlab/core.py

bsipocz · 2025-09-13T00:30:19Z

astroquery/noirlab/core.py

+        return response
+
+    def core_fields(self, hdu=False, cache=True):
+        """List the available CORE fields for file or HDU searches.


If there are any outside docs page that describes what CORE fields mean, it would be useful to mentioned it here.

I think so, I will take a look.

bsipocz · 2025-09-13T00:35:07Z

astroquery/noirlab/core.py

+        response.raise_for_status()
+        return self._response_to_table(response.json())
+
+    def retrieve(self, fileid):


rather than retrieve, it would be more consistent to call this get_image or something similar that we already have elsewhere in astroquery.

(we have a couple of variation on it though, and here I think we don't distinguish between file content type (if it's an image vs spectrum -- though technically everything is an image? -- so you could call it get_file)

I summon @keflavich to see if he has some better insight.

I have renamed this to get_file in the latest commit, but if another name is preferred, it is easy to change.

astroquery/noirlab/core.py

docs/noirlab/noirlab.rst

weaverba137 · 2025-09-13T16:57:54Z

Thank you, I was planning to pick this up next week anyway, so this is good timing.

weaverba137 · 2025-09-17T21:27:11Z

Commit 64b84bf addresses some of the review comments. Others will be addressed soon.

There is an issue that needs to be investigated on the NOIRLab side though. Since I last tested in July, when metadata fields are requested in a query, e.g. md5sum, the field is returned, but a duplicate field with header file:md5sum is also returned. This behavior was unexpected both by me and the developer I mentioned this to.

weaverba137 · 2025-09-17T21:34:47Z

Also, I have no clue why the noirlab.rst is causing a RtD error, because the headers are consistent with other files.

bsipocz · 2025-09-18T03:27:58Z

Also, I have no clue why the noirlab.rst is causing a RtD error, because the headers are consistent with other files.

That error rings a bell, and will be related to the generated API docs. I would recommend ignoring it for now and I'll have a closer look once this is ready to go in.

weaverba137 added 4 commits June 26, 2025 15:02

add files from NOAO fork

d6345c6

add noirlab.rst to index

e6a7dac

update change log

dd77a3f

update url and tests

9b093d1

fix remote data import

e5c647b

weaverba137 added 5 commits June 26, 2025 16:27

fix doc errors

a02dbef

add local test

bb3bbef

adding offline tests

14834a4

ongoing test refactoring

13d7686

fix test patching

f090357

weaverba137 added 16 commits June 27, 2025 14:09

adding more tests

f8fc5cf

fix docstring

6a70c46

adding more tests

6eaacb6

activate additional tests and add placeholders

2f8816e

fix style issue

714c2f0

working on doc strings

25f879f

fix pformat issues

298a69f

update docs

c703673

add test coverage for some corner cases

038a5f6

fix style & add placeholders

7929eba

tweak formatting

e0c421f

update and sync docs

f63e091

activate hdu metadata tests

29b2437

all tests now active

3894c16

fix remote data error

afa21af

fix doc compilation errors

708dbb4

weaverba137 marked this pull request as ready for review July 17, 2025 23:51

bsipocz added New Service noirlab labels Aug 12, 2025

bsipocz added this to the v0.4.11 milestone Aug 12, 2025

bsipocz reviewed Aug 12, 2025

View reviewed changes

bsipocz reviewed Sep 13, 2025

View reviewed changes

updates based on initial review

64b84bf

fix style check

f643b79

bsipocz modified the milestones: v0.4.11, 0.4.12 Sep 18, 2025

Uh oh!

Access to NOIRLab Astro Data Archive #3359

Are you sure you want to change the base?

Access to NOIRLab Astro Data Archive #3359

Uh oh!

Conversation

weaverba137 commented Jun 26, 2025 • edited by bsipocz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weaverba137 commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

weaverba137 commented Jun 26, 2025

Uh oh!

bsipocz commented Jun 27, 2025

Uh oh!

weaverba137 commented Jul 17, 2025

Uh oh!

weaverba137 commented Jul 17, 2025

Uh oh!

bsipocz commented Jul 18, 2025

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

weaverba137 commented Aug 13, 2025

Uh oh!

bsipocz commented Aug 13, 2025

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Uh oh!

weaverba137 commented Sep 13, 2025

Uh oh!

weaverba137 commented Sep 17, 2025

Uh oh!

weaverba137 commented Sep 17, 2025

Uh oh!

bsipocz commented Sep 18, 2025

Uh oh!

Uh oh!

weaverba137 commented Jun 26, 2025 •

edited by bsipocz

Loading

weaverba137 commented Jun 26, 2025 •

edited

Loading

codecov bot commented Jun 26, 2025 •

edited

Loading