ESA: abstract EsaTap using PyVO, HST and ISLA refactored, EMDS module and EinsteinProbe module#3511
Conversation
|
Thanks for the updates @pdmElisa ! |
|
It seems that the workflows are waiting for the approval of a maintainer. I cannot see any button to launch them, @bsipocz . Can you please execute them to verify that the changes are ok? |
|
Hi @bsipocz ! We are in the process of creating additional PRs from other ESA modules soon, based on the code included in this branch. Sorry for the rush, but it would be great if we could iterate on it and try to merge it as soon as possible. FYI, these future PRs will include a new module, and we will request a formal release by last week of March / first week of April. @pdmElisa and myself will fix quickly the changes you request. |
|
@jespinosaar - I'll try to get back to this one next week. The diff looks enormous, so it hasn't got to the top of the quick-things-to-review list yet. |
|
(But I also noticed looking at the cron test jobs that the current integral module is very badly broken, so that definitely bumped up the priority on this one) |
|
Hi @bsipocz! |
213f26d to
83ffebe
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3511 +/- ##
==========================================
+ Coverage 72.66% 73.10% +0.43%
==========================================
Files 219 224 +5
Lines 20478 20839 +361
==========================================
+ Hits 14880 15234 +354
- Misses 5598 5605 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Dear @bsipocz , I have rebased this PR and added some changes to increase code-coverage and ensure sphinx is working as expected. In addition to this, the Archives are available again, so you can now execute all the remote tests. As we already commented, it would be good if we could review this soon, as we will be using this code to generate other modules as well. We will be waiting for your feedback. Many thanks for your patience! 😄 |
|
Dear @bsipocz , We would like to merge now another module from a different mission. This should be available at the end of this month (presumably with a released version, if not we can work with a pre-released). The problem here is that we are basing this development on what we have prepared for this PR. How should we proceed? Do you prefer to have this merged and then merge the new module in a different PR? Or should I include that code in this PR? Thanks in advance and sorry for insisting! |
|
Hi @bsipocz , @adamginsburg , |
|
Hi @bsipocz , @adamginsburg , Kind regards, |
bsipocz
left a comment
There was a problem hiding this comment.
Before starting a detailed review I noticed that there are lots of test failures. Could you please have a look at them? Here is a very brief summary; I haven't yet look into the tracebacks.
pytest astroquery docs -P esa.emds,esa.hubble,esa.integral,esa.utils,esa.xmm_newton -R
================================================= test session starts ==================================================
platform darwin -- Python 3.12.1, pytest-9.0.2, pluggy-1.6.0
cachedir: .tox/test-online/.pytest_cache
Running tests with astroquery version 0.4.12.dev356+gf09e2fb37_testrun.
Running tests in astroquery docs.
Date: 2026-03-17T11:28:39
Platform: macOS-12.5.1-arm64-arm-64bit
Executable: /Users/bsipocz/munka/devel/astroquery/.tox/test-online/bin/python
Full Python Version:
3.12.1 (main, Dec 22 2023, 11:59:08) [Clang 14.0.0 (clang-1400.0.29.202)]
encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8
byteorder: little
float info: dig: 15, mant_dig: 15
Package versions:
Numpy: 2.4.3
Matplotlib: 3.10.8
Astropy: 7.2.0
regions: not available
pyVO: 1.8.1
mocpy: not available
astropy-healpix: not available
vamdclib: not available
Using Astropy options: remote_data: any.
rootdir: /Users/bsipocz/munka/devel/astroquery
configfile: pyproject.toml
plugins: astropy-0.11.0, mock-3.15.1, remotedata-0.4.1, timeout-2.4.0, custom-exit-code-0.3.0, filter-subpackage-0.2.0, astropy-header-0.2.2, hypothesis-6.151.9, doctestplus-1.7.1, arraydiff-0.6.1, dependency-0.6.1, rerunfailures-16.1, cov-7.0.0
timeout: 300.0s
timeout method: signal
timeout func_only: False
collected 240 items
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/emds/einsteinprobe/test/test_einsteinprobe_remote.py . [ 0%]
...FFFFFF [ 4%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/emds/einsteinprobe/test/test_einsteinprobe_tap.py . [ 4%]
.................... [ 12%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/emds/tests/test_emds_remote.py .....FFFFFFE [ 17%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/emds/tests/test_emds_tap.py ................. [ 24%]
.. [ 25%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/hubble/tests/test_esa_hubble.py ............. [ 30%]
................................. [ 44%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/hubble/tests/test_esa_hubble_remote.py ...... [ 47%]
......... [ 50%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/integral/tests/test_isla_remote.py .....F..F. [ 55%]
.F [ 55%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/integral/tests/test_isla_tap.py ............. [ 61%]
.............E.......... [ 71%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/utils/tests/test_utils.py ................... [ 79%]
........... [ 83%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/xmm_newton/tests/test_xmm_newton.py ......... [ 87%]
................. [ 94%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/xmm_newton/tests/test_xmm_newton_remote.py F. [ 95%]
...... [ 97%]
../../docs/esa/emds/einsteinprobe/einsteinprobe.rst F [ 98%]
../../docs/esa/emds/emds.rst F [ 98%]
../../docs/esa/hubble/hubble.rst F [ 99%]
../../docs/esa/integral/integral.rst F [ 99%]
../../docs/esa/xmm_newton/xmm_newton.rst F [100%]
======================================================== ERRORS ========================================================
|
Hi @bsipocz , and many thanks for looking into this and your support. I have modified the tests and the documentation, and now everything seems to be working as expected. Could you please test it again? And let me know if you find more issues. Many thanks! |
bsipocz
left a comment
There was a problem hiding this comment.
Thank you @jespinosaar of the continued patience with this.
I have a number of review comments, most of them about API consistency and namespaces., and a couple of minor nitpicking items that doesn't need to be addressed.
The only blockers is to iterate on the names/namespaces -- you don't have to follow my suggestions but I would love to see your thinking on them.
| super().__init__(auth_session=auth_session, tap_url=tap_url) | ||
| self.conf = conf | ||
|
|
||
| def get_products(self, obs_id=None, *, columns=None, custom_filters=None, |
There was a problem hiding this comment.
Some API consistency could help here I think. We do have get_product() elsewhere (e.g. euclid and jwst modules), and agruments are called observation_id or product_id or artifact_id. Could we have some renaming to have a bit more consistency?
Also, in the other similar methods all optional arguments are mandatory kwargs, so please move the * one place earlier
There was a problem hiding this comment.
This method aims to retrieve a list of products, this is why it is called get_products. If you want to keep the consistency, I can change it to get_product, but I think this may be less intuitive. I would like to know your opinion on this.
The * has been moved.
There was a problem hiding this comment.
ok, let's keep the plural then.
But I think the parameter renames should still be considered for better consistency.
There was a problem hiding this comment.
I have renamed obs_id to observation_id
| elif isinstance(columns, list): | ||
| for r in required: | ||
| if r not in columns: | ||
| columns.append(r) |
There was a problem hiding this comment.
bit overoptimisation, but what about making columns (internally) a set, and then you can just add required instead of looping over?
There was a problem hiding this comment.
I have applied your suggestion
| # Build an obs_id filter if provided | ||
| obs_filter = None | ||
| if obs_id: | ||
| obs_filter = "obs_id = '{0}'".format(obs_id) |
There was a problem hiding this comment.
| obs_filter = "obs_id = '{0}'".format(obs_id) | |
| obs_filter = f"obs_id = '{obs_id}'" |
There was a problem hiding this comment.
I applied your suggestion
astroquery/esa/utils/utils.py
Outdated
| REQUEST_PARAMETERS = {} | ||
| """ | ||
|
|
||
| def __init__(self, auth_session=None, tap_url=None): |
There was a problem hiding this comment.
| def __init__(self, auth_session=None, tap_url=None): | |
| def __init__(self, *, auth_session=None, tap_url=None): |
There was a problem hiding this comment.
The asterisk has been added
astroquery/esa/utils/utils.py
Outdated
|
|
||
| Returns | ||
| ------- | ||
| A pyvo.dal.TAPService object connected to {ESA_ARCHIVE_NAME} TAP |
There was a problem hiding this comment.
| A pyvo.dal.TAPService object connected to {ESA_ARCHIVE_NAME} TAP | |
| A `~pyvo.dal.TAPService` object connected to {ESA_ARCHIVE_NAME} TAP |
There was a problem hiding this comment.
I have added your suggestion
|
|
||
| def __create_sql_criteria(self, filters): | ||
| """ | ||
| Create the SQL clause associated to the query_criteria filters |
There was a problem hiding this comment.
is this vanilla SQL or ADQL? if the latter, please rename
There was a problem hiding this comment.
It is an ADQL query to the TAP, I have renamed it
astroquery/esa/utils/utils.py
Outdated
| """ | ||
| # For exact searches in not integer values, check the number is | ||
| # within a threshold of 1e-5 | ||
| threshold = 1e-5 |
There was a problem hiding this comment.
could/should this threshold be configurable?
(I really just wonder, even if it could be, no need to address or change in this PR but instead leave it for a possible future enhancement)
There was a problem hiding this comment.
I have modified it as a parameter for the class, so it can be configured when instantiating it.
|
I think I have addressed all your comments, @bsipocz . Please let me know if further changes need to be integrated. With respect to the importance of the EMDS, I think it is important to keep it as a separate module, as this will be the entry point for different archives, providing a multi-mission approach. We had the same discussion internally, about einsteinprobe being inside emds or as an extra module inside "esa" folder. We finally added Einstein Probe inside this module, to show that this mission is part of this infrastructure. If, at some point, a mission with a existing module under esa folder is migrated to EMDS, we will discuss with you on the best approach. Maybe we can keep it separately for existing modules. But, for sure, we will keep you informed. |
OK, that sounds reasonable, we definitely need to keep |
Ok, maybe we can have a longer discussion later, yes, and decide the best approach. Many thanks! In the meantime, I think all the comments are now addressed |
|
Just added a minor improvement, to verify if a radius has been defined as a quantity. Thanks @bsipocz ! |
bsipocz
left a comment
There was a problem hiding this comment.
Thank you so much @jespinosaar for your patience with this. Let's merge with the aim of a tagged release by the end of the month, or middle May the latest. So any PRs that are follow-ups would be best suited to land before the release.
I have noticed that some of the remote tests seems to run into issues, but they are not deterministic and sometimes pass for the second test run. It maybe something worth looking into in a follow-up.
Dear Astroquery team,
We come from PR 3501: #3501
As part of the migration to PyVO, we have generated a more general class called EsaTap, extending PyVO capabilities with custom code (authentication, parameters in the request, methods to query tables with specific filters...). We will be using this class to extend ESA modules, so the common code will remain under EsaTap and the mission-specific methods will be defined in their specific modules. As a reference, we have already applied this approach to eHST and ISLA.
In addition to this, we have generated a new module called ESDC Multi-Mission Data Services (EMDS). Then, inside it, we have generated the module for Einstein Probe module, that depends on the EMDS one.
Please let us know if this is ok with you.
Happy to receive your feedback and implement any change you require!
Kind regards,
@jespinosaar and @pdmElisa
cc @esdc-esac-esa-int