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.90722% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.20%. Comparing base (82cab62) to head (708dbb4).
⚠️ Report is 26 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3359      +/-   ##
==========================================
+ Coverage   70.07%   70.20%   +0.13%     
==========================================
  Files         232      234       +2     
  Lines       19893    19990      +97     
==========================================
+ Hits        13940    14034      +94     
- Misses       5953     5956       +3

☔ 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.

bsipocz · 2025-08-12T19:41:02Z

astroquery/noirlab/core.py

+__all__ = ['NOIRLab', 'NOIRLabClass']  # specifies what to import
+
+
+@async_to_sync


We may not want to use async_to_sync any more, see further comments about the async_job kwarg

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?

bsipocz · 2025-08-12T19:45:18Z

astroquery/noirlab/core.py

+        response = self._request('GET', url, timeout=self.TIMEOUT, cache=cache)
+        return response.json()
+
+    def query_region(self, coordinate, radius=0.1, hdu=False, cache=True):


Please use mandatory kwargs for optional parameter

Suggested change

def query_region(self, coordinate, radius=0.1, hdu=False, cache=True):

def query_region(self, coordinate, *, radius=0.1, hdu=False, cache=True):

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).

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(...)

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!

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

		__all__ = ['NOIRLab', 'NOIRLabClass'] # specifies what to import


		@async_to_sync

	def query_region(self, coordinate, radius=0.1, hdu=False, cache=True):
	def query_region(self, coordinate, *, radius=0.1, hdu=False, cache=True):

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!

bsipocz Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

bsipocz Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

bsipocz Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

bsipocz Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

bsipocz Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

weaverba137 commented Aug 13, 2025

Uh oh!

bsipocz commented Aug 13, 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