Skip to content

Add ESO catalogue functions#3531

Open
ashleythomasbarnes wants to merge 16 commits intoastropy:mainfrom
eso:develop-catalogues
Open

Add ESO catalogue functions#3531
ashleythomasbarnes wants to merge 16 commits intoastropy:mainfrom
eso:develop-catalogues

Conversation

@ashleythomasbarnes
Copy link
Copy Markdown
Contributor

@ashleythomasbarnes ashleythomasbarnes commented Feb 13, 2026

We've added functions in the same structure similar to the others to query the ESO catalogues:

  • list_catalogues -> lists all available catalogues
  • query_catalogue -> query results for a specific catalogue with constraints

Docs are coming in another PR.

-- Thanks!
@juanmcloaiza @szampier @almicol

@ashleythomasbarnes ashleythomasbarnes changed the title Add ESO catalogue functions (#8) Add ESO catalogue functions Feb 13, 2026
Comment on lines +16 to +20
eso
^^^

- Add functionality to list and query ESO catalogues. [#8]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I seem to remember that this file is normally edited by the astroquery maintainers

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this is correct, preferably the changelog entry is added in the PR by the PR author.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you need more here...

@szampier
Copy link
Copy Markdown
Contributor

General comment: it’s a bit difficult to focus on the new feature since the PR also contains many formatting changes. Splitting these into a separate commit or PR would make the review easier.

@bsipocz bsipocz added the eso label Feb 13, 2026
@bsipocz bsipocz added this to the 0.4.12 milestone Feb 13, 2026
Copy link
Copy Markdown
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've started reviewing this, but the code style changes are a bit overwhelming, so I stopped when I got to the actual changes in core.py.

Could you please remove any of the style changes (and anyway, please don't force short line length on code) and force push back the actual code changes?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is not needed any more, please remove.

Comment on lines -19 to +21
tap_url = _config.ConfigItem(
tap_obs_url = _config.ConfigItem(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is technically an API change; but for now it is acceptable as we haven't yet released the ESO refactor work.

Comment on lines +36 to +52
from ..exceptions import (
RemoteServiceError,
LoginError,
NoResultsWarning,
MaxResultsWarning,
)
from ..query import QueryWithLogin
from ..utils import schema
from .utils import _UserParams, raise_if_coords_not_valid, _reorder_columns, \
_raise_if_has_deprecated_keys, _build_adql_string, \
DEFAULT_LEAD_COLS_PHASE3, DEFAULT_LEAD_COLS_RAW
from .utils import (
_UserParams,
raise_if_coords_not_valid,
_reorder_columns,
_raise_if_has_deprecated_keys,
_build_adql_string,
DEFAULT_LEAD_COLS_PHASE3,
DEFAULT_LEAD_COLS_RAW,
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wish these type of style refactorings would not be mixed into feature branches.

Also, we do allow longer lines in astroquery, up to 120 characters, so please don't apply these black type spreads over many lines.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ashleythomasbarnes
Copy link
Copy Markdown
Contributor Author

Indeed this should have been a draft PR like we agreed (sorry @szampier)... How should we proceed - shall I revert to draft and make changes, or okay to keep as-in?

@ashleythomasbarnes
Copy link
Copy Markdown
Contributor Author

Okay, I cleaned up my mess - it should be clearer now. Let me know how you want to proceed...

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 13, 2026

Codecov Report

❌ Patch coverage is 75.00000% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.69%. Comparing base (de342ea) to head (86877ae).
⚠️ Report is 30 commits behind head on main.

Files with missing lines Patch % Lines
astroquery/eso/core.py 73.68% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3531      +/-   ##
==========================================
- Coverage   72.71%   72.69%   -0.02%     
==========================================
  Files         219      219              
  Lines       20473    20540      +67     
==========================================
+ Hits        14886    14931      +45     
- Misses       5587     5609      +22     

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

@ashleythomasbarnes
Copy link
Copy Markdown
Contributor Author

Just to let you know @bsipocz - I spoke with @juanmcloaiza and @szampier this morning, and we're happy for you to continue the review. I will be away from 1st March until ~ 7th April, so it would be great if we could get this wrapped up by then, otherwise @juanmcloaiza has offered to take over any needed changes whilst I'm gone. Thanks!

updates so query_catalogue and list_catalogues work for both TAP 1.0 and TAP 1.1 - dealing with "safcat." prefix
@ashleythomasbarnes
Copy link
Copy Markdown
Contributor Author

Sorry @bsipocz, one final commit before I leave tomorrow. This was a very minor one such that @almicol and co can move forward with TAP 1.1 for tap_cap -- still okay now to review when you can. @juanmcloaiza will take over if there is anything urgent while I'm gone.

Copy link
Copy Markdown
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I see a lot of test failures in this PR for the remote tests. Could you please have a look and fix them before we do more reviews?

The easiest is to test locally with tox -e test-online -- -P eso

Copy link
Copy Markdown
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some naming and API issues that would be nice to address, otherwise the code looks good.

I also still see 3 test failures due to maxrec warnings, but they are also present on the main branch, so this PR doesn't have to fix them.

Authentication must be performed beforehand via
:meth:`astroquery.eso.EsoClass.login`. Authenticated queries
may be slower. Default is ``False``.
which_tap : {"tap_obs", "tap_cat"}, optional
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be named better, more with a descriptor than a question?
E.g tap_endpoint

table_to_return = Table()
tap_service = self.tap(authenticated)
tap_service = self.tap(authenticated, which_tap=which_tap)
table_to_return = self._try_download_pyvo_table(query, tap_service)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't noticed in the previous review, but I think this method should not be called download as we don't really download a file containing the table but keeping everything in memory. (everywhere else download kind of indicates some file download functionality.

(doesn't have to be part of this PR, but would be nice to change it before a release)

f"\nNumber of records present in the table {table_name}:\n{num_records}\n")

@unlimited_maxrec
def list_catalogues(self, all_versions: bool = False, cache: bool = True) -> List[str]:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make optional params kwarg only

Suggested change
def list_catalogues(self, all_versions: bool = False, cache: bool = True) -> List[str]:
def list_catalogues(self, *, all_versions: bool = False, cache: bool = True) -> List[str]:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and I hate to say this as I love the 'u's and proper spellings, but it's better for the user experience to have a more uniform API. So, please change the method name to list_catalogs.
British spelling can stay in the narrative docs and comments.

authenticated=authenticated)
return self._query_on_allowed_values(user_params)

def query_catalogue(self,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, please change this to query_catalog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants