Conversation
- Introduced new queries for ASM tables and columns in the ESO module. - Added tests for listing ASM instruments and querying ASM data. - Implemented error handling for unknown columns in ASM queries. - Enhanced remote tests to validate ASM query payloads.
| l_res = list(res) | ||
| l_res = list(map(lambda x: x.split(".", 1)[1], l_res)) | ||
|
|
||
| return l_res |
There was a problem hiding this comment.
could be probably rewritten as:
return [x.split(".", 1)[1] for x in res]
assuming that res is an iterable.
|
I'm not sure how you plan to do this, but please make sure that you don't submit any changes outside of |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for querying Astronomical Site Monitor (ASM) data from the ESO archive, introducing list_asm() and query_asm() methods that parallel the existing list_instruments() and query_instrument() functionality. However, the PR includes several unrelated changes to CI workflows and test configurations that significantly impact its scope and quality.
Changes:
- Added
list_asm()andquery_asm()methods to query ASM tables (e.g., ambient conditions, weather data) - Enhanced
list_column()to optionally include column descriptions - Added GitHub Actions workflow for automated fork synchronization with upstream
- Modified CI workflows to trigger on a 'develop' branch
- Narrowed test scope configuration to only test the ESO module
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| astroquery/eso/core.py | Core implementation of list_asm() and query_asm() methods, plus _get_table_columns() helper and updated list_column() |
| astroquery/eso/tests/test_eso.py | Unit tests for new ASM functionality with test data file mappings |
| astroquery/eso/tests/test_eso_remote.py | Remote integration test for ASM query payload generation |
| astroquery/eso/testing/test_query_asm.ipynb | Jupyter notebook demonstrating ASM query usage with examples |
| tox.ini | CRITICAL ISSUE: Test scope narrowed to only ESO module (should test all of astroquery) |
| setup.cfg | CRITICAL ISSUE: Test paths restricted to ESO module only (should cover entire project) |
| .github/workflows/sync_fork.yml | New workflow for fork synchronization (unrelated to PR purpose) |
| .github/workflows/ci_tests.yml | Added 'develop' branch trigger (unrelated to PR purpose) |
| .github/workflows/ci_devtests.yml | Added 'develop' branch trigger (unrelated to PR purpose) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Returns | ||
| ------- | ||
| asm_list : list of strings | ||
| cache : bool | ||
| Deprecated - unused. | ||
| """ |
There was a problem hiding this comment.
The docstring has incorrect structure. The 'cache' parameter should be documented in a 'Parameters' section, not mixed with 'Returns'. The current format will not render properly in documentation tools like Sphinx.
| - develop | ||
| tags: | ||
| - '*' | ||
| pull_request: | ||
| branches: | ||
| - main | ||
| - develop |
There was a problem hiding this comment.
The addition of a 'develop' branch to CI workflow triggers is not mentioned in the PR description, which only describes ASM query functionality. This appears to be an infrastructure change unrelated to the main purpose of this PR and should either be in a separate PR or explained in the PR description.
| docs/eso | ||
| astroquery/eso |
There was a problem hiding this comment.
The testpaths configuration has been narrowed to only test the ESO module instead of the entire astroquery package. This change should not be in this PR as it affects the entire project's testing strategy. The original configuration tested all of astroquery and its docs, which is the appropriate scope. This appears to be a debugging configuration that was accidentally committed.
| docs/eso | |
| astroquery/eso | |
| docs | |
| astroquery |
| "select table_name from TAP_SCHEMA.tables where schema_name='asm' order by table_name": | ||
| "query_list_asm.csv", | ||
|
|
||
| "select column_name, datatype, xtype, unit from TAP_SCHEMA.columns " | ||
| "where table_name = 'asm.ambient_lasilla'": | ||
| "query_asm_columns_ambient_lasilla.csv", | ||
|
|
||
| "select temperature, humidity from asm.ambient_lasilla where temperature > 10": | ||
| "query_asm_ambient_lasilla.csv", |
There was a problem hiding this comment.
Three new test data files are referenced in the test configuration but are not included in this PR: 'query_list_asm.csv', 'query_asm_columns_ambient_lasilla.csv', and 'query_asm_ambient_lasilla.csv'. These files must be added to 'astroquery/eso/tests/data/' directory for the tests to pass. Without these files, the test_list_asm, test_query_asm, and test_query_asm_unknown_column tests will fail.
| run: | | ||
| cd ${{ github.workspace }} | ||
| git config --global user.email "jcarmona@eso.org" |
There was a problem hiding this comment.
The workflow file includes hardcoded personal email 'jcarmona@eso.org' in the git configuration. This should either use a generic project email or be configured as a repository secret/variable to avoid exposing personal information and to allow for easier maintenance when ownership changes.
| run: | | |
| cd ${{ github.workspace }} | |
| git config --global user.email "jcarmona@eso.org" | |
| env: | |
| GIT_USER_EMAIL: ${{ secrets.GIT_USER_EMAIL }} | |
| run: | | |
| cd ${{ github.workspace }} | |
| git config --global user.email "${GIT_USER_EMAIL}" |
| - develop | ||
| tags: | ||
| - '*' | ||
| pull_request: | ||
| branches: | ||
| - main | ||
| - develop |
There was a problem hiding this comment.
The addition of a 'develop' branch to CI workflow triggers is not mentioned in the PR description, which only describes ASM query functionality. This appears to be an infrastructure change unrelated to the main purpose of this PR and should either be in a separate PR or explained in the PR description.
| def query_asm( | ||
| self, | ||
| asm_table: str, *, | ||
| help: bool = False, | ||
| columns: Union[List, str] = None, | ||
| column_filters: Optional[dict] = None, | ||
| maxrec: int = None, | ||
| **kwargs, |
There was a problem hiding this comment.
The query_asm method lacks several parameters present in query_instrument (cone_ra, cone_dec, cone_radius, order_by, order_by_desc) but documents order_by and order_by_desc as being available through **kwargs. While cone search might not be applicable for ASM data, the inconsistency in how parameters are handled (some explicit vs some in **kwargs) makes the API less clear. Consider making order_by and order_by_desc explicit parameters like in query_instrument for consistency and better API clarity.
| table_name = _EsoNames.asm_table(asm_table) | ||
|
|
||
| if help: | ||
| self.list_column(table_name, include_description=True) |
There was a problem hiding this comment.
When help=True is used in query_asm, the function calls list_column with include_description=True to show descriptions. However, in _query_on_allowed_values (line 441), when print_help is True, it calls list_column WITHOUT include_description, defaulting to False. This creates an inconsistency: query_asm(help=True) shows descriptions, but query_instrument(help=True) does not. For consistency across the API, both should behave the same way.
| self.list_column(table_name, include_description=True) | |
| self.list_column(table_name) |
|
|
||
| @unlimited_maxrec | ||
| @deprecated_renamed_argument('cache', None, since='0.4.12') | ||
| def list_asm(self, cache=True) -> List[str]: |
There was a problem hiding this comment.
The list_surveys method uses keyword-only arguments (*, cache=True) on line 383, but the newly added list_asm method (line 361) does not follow this pattern and uses positional-or-keyword argument (cache=True). For consistency with list_surveys, consider using keyword-only arguments. Note that list_instruments also doesn't use keyword-only, suggesting this might be an area where the codebase itself is inconsistent.
| def list_asm(self, cache=True) -> List[str]: | |
| def list_asm(self, *, cache=True) -> List[str]: |
| !cov: pytest --pyargs astroquery.eso {toxinidir}/docs/eso {env:PYTEST_ARGS} {posargs} | ||
| cov: pytest --pyargs astroquery.eso {toxinidir}/docs/eso --cov astroquery.eso --cov-config={toxinidir}/setup.cfg {env:PYTEST_ARGS} {posargs} | ||
| # For remote tests, we re-run the failures to filter out at least some of the flaky ones. | ||
| # We use a second pytest run with --last-failed as opposed to --rerun in order to rerun the | ||
| # failed ones at the end rather than right away. | ||
| online: pytest --pyargs astroquery {toxinidir}/docs {env:PYTEST_ARGS_2} {posargs} | ||
| online: pytest --pyargs astroquery.eso {toxinidir}/docs/eso {env:PYTEST_ARGS_2} {posargs} |
There was a problem hiding this comment.
The test scope has been narrowed to only test the ESO module instead of the entire astroquery package. This change should not be in this PR as it affects the entire project's testing strategy, not just the ASM functionality. This appears to be a debugging or development configuration that was accidentally committed. The original configuration tested all of astroquery, which is the appropriate scope for the main test suite.
Draft of query_asm and list_asm functionality, which works very similar to the e.g.
query_instrumentfunction, but queries the asm.XXX tables instead of the ist.XXX tables.