ESA Integral Science Legacy Archive Astroquery module #3154

jespinosaar · 2024-12-18T15:31:02Z

Dear Astroquery team,

This PR aims to integrate a new module for the ESA Integral Science Legacy Archive. The main difference with other modules is that this is the first integration of an ESA module using PyVO, instead of the deprecated astroquery.utils.tap module. We have developed a fully functional module that can be used as reference to migrate the rest. Please let us know your thoughts and we will iterate on this to merge it. Thanks for your support!

Would it be possible to create a release, right after this module has been merged? Thank you very much.

Kind regards,

@jespinosaar

cc @esdc-esac-esa-int

pep8speaks · 2024-12-18T15:31:11Z

Hello @jespinosaar! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2025-01-22 12:29:15 UTC

jespinosaar · 2024-12-18T17:01:11Z

Ok, the tests are not working in the CI/CD environment. It seems that, at some point, it tries to connect to the URL. I would need some support on this. Maybe I should define the tap parameter as a property, similar to what is done for other modules. What do you think, @bsipocz ? Do you recommend any other alternative?

codecov · 2024-12-19T08:47:18Z

Codecov Report

Attention: Patch coverage is 89.86784% with 46 lines in your changes missing coverage. Please review.

Project coverage is 68.18%. Comparing base (fbf55b0) to head (f181ac8).
Report is 103 commits behind head on main.

Files with missing lines	Patch %	Lines
astroquery/esa/integral/core.py	89.25%	29 Missing ⚠️
astroquery/esa/utils/utils.py	89.57%	17 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3154      +/-   ##
==========================================
+ Coverage   67.49%   68.18%   +0.68%     
==========================================
  Files         232      231       -1     
  Lines       18446    19048     +602     
==========================================
+ Hits        12450    12987     +537     
- Misses       5996     6061      +65

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

jespinosaar · 2024-12-19T09:00:05Z

I applied the changes I commented before and it seems all checks are passing now! So we can now check if everything is ok, iterate on changes and, when possible, merge it. Please let me know your thoughts and thanks in advance for the feedback!

bsipocz · 2024-12-19T19:38:12Z

Thanks @jespinosaar. I'll bump this into the new year as it's unrealistic to promise a review before we're off for a break.

jespinosaar · 2024-12-19T20:35:31Z

Of course, no problem! thanks @bsipocz and Merry Christmas!

jespinosaar · 2025-01-07T14:31:04Z

Hi again, and happy new year! I just fixed some conflicts that appeared after some pull requests were merged.

bsipocz

Thank you @jespinosaar!

Could you please add some remote_data tests as I see everything is mocked at the moment or skipped when part of the documentation.

Beyond that I had only a few comments mostly focusing on concentrating the API into fewer new methods.

bsipocz · 2025-01-17T04:05:51Z

docs/esa/integral/integral.rst

@@ -0,0 +1,357 @@
+.. doctest-skip-all


Why are the tests skipped in here?

I removed that line.

bsipocz · 2025-01-17T04:08:19Z

astroquery/esa/utils/utils.py

Some test coverage would be nice for these functionalities

I have added the tests to almost all the features.

bsipocz · 2025-01-17T05:13:14Z

astroquery/esa/utils/utils.py

+    return response
+
+
+def plot_result(x, y, x_title, y_title, plot_title, *, error_x=None, error_y=None, log_scale=False):


We don't usually do any add-on to data access functionality, such as plotting, so I'm a bit hesitant about these ones

Totally agree, we should use astroquery to retrieve data only. I removed them.

bsipocz · 2025-01-17T05:15:14Z

astroquery/esa/utils/utils.py

+        return super()._request(method, url, **kwargs)
+
+
+def get_coord_input(value, msg):


most of these are generic enough, so may need to be cleaned up/consolidated. E.g. why not just use commons.parse_coordinates in the integral module?

bsipocz · 2025-01-17T05:37:00Z

astroquery/esa/integral/core.py

+        except Exception as e:
+            log.error('No science windows have been found with these inputs. {}'.format(e))
+
+    def get_timeline(self, ra, dec, *, radius=14, plot=False, plot_revno=False, plot_distance=False):


I wonder if this could take a SkyCoord input instead of the floats

bsipocz · 2025-01-17T05:39:45Z

astroquery/esa/integral/core.py

+            query = conf.ISLA_EPOCH_QUERY.format(instrument_oid, band_oid)
+        return self.query_tap(query)
+
+    def get_long_term_timeseries(self, target_name, *, instrument=None, band=None, plot=False):


I wonder if the get_* and download* methods can be merged into one where the download is just a parameter, after all they are almost identical? Would that make sense?

They were providing different outputs, but it is true that the valid ones are retrieved using download methods. So, I have removed the original get_* methods and renamed the download_* to get_*, plus other features.

jespinosaar · 2025-01-17T16:29:05Z

Many thanks for your feedback, @bsipocz ! I will work on them

jespinosaar · 2025-01-21T15:34:33Z

Just answered some of your comments. Now I am working on the remote tests and the ones for utils.py.

jespinosaar · 2025-01-22T18:42:04Z

I have pushed the code to address the rest of your comments, @bsipocz . Please let me know if further changes are required.

It would be great if we could have a pre-release version containing this new module.

Thanks in advance!

bsipocz · 2025-01-23T04:32:15Z

Unfortunately, I see a bunch of test failures (I don't see why they didn't fail in CI), e.g the ones below. But also I see a lot of unclosed file warnings in the remote tests:

However, while this may be a local to me issue I also run into a bunch of data files downloaded into the main repo directory during the tests as opposed to a tempdir. Doing that may solve the unclosed file warnings out of the box.

======================================================= FAILURES =======================================================
____________________________________ TestIntegralTap.test_get_short_term_timeseries ____________________________________

self = <astroquery.esa.integral.tests.test_isla_tap.TestIntegralTap object at 0x13ba33bf0>
instrument_band_mock = <MagicMock name='get_instrument_band_map' id='5302829664'>
epoch_mock = <MagicMock name='get_epochs' id='5302833504'>
download_mock = <MagicMock name='download_file' id='5302837440'>

    @patch('astroquery.esa.integral.core.pyvo.dal.TAPService.capabilities', [])
    @patch('astroquery.esa.utils.utils.download_file')
    @patch('astroquery.esa.integral.core.IntegralClass.get_epochs')
    @patch('astroquery.esa.integral.core.IntegralClass.get_instrument_band_map')
    def test_get_short_term_timeseries(self, instrument_band_mock, epoch_mock, download_mock):
    
        instrument_band_mock.return_value = mocks.get_instrument_bands()
        epoch_mock.return_value = {'epoch': ['time']}
        download_mock.return_value = data_path('tar_file.tar')
    
        isla = IntegralClass()
        mock_instrument_bands(isla_module=isla)
        st_timeseries_list_extracted = isla.get_short_term_timeseries(target_name='target',
                                                                      band='b1', epoch='time')
>       assert len(st_timeseries_list_extracted) == 3
E       TypeError: object of type 'NoneType' has no len()

astroquery/esa/integral/tests/test_isla_tap.py:491: TypeError
___________________________________________ TestIntegralTap.test_get_spectra ___________________________________________

self = <astroquery.esa.integral.tests.test_isla_tap.TestIntegralTap object at 0x13ba306e0>
instrument_band_mock = <MagicMock name='get_instrument_band_map' id='5302779984'>
servlet_mock = <MagicMock name='execute_servlet_request' id='5302687152'>
epoch_mock = <MagicMock name='get_epochs' id='5302783776'>
download_mock = <MagicMock name='download_file' id='5302788576'>

    @patch('astroquery.esa.integral.core.pyvo.dal.TAPService.capabilities', [])
    @patch('astroquery.esa.utils.utils.download_file')
    @patch('astroquery.esa.integral.core.IntegralClass.get_epochs')
    @patch('astroquery.esa.utils.utils.execute_servlet_request')
    @patch('astroquery.esa.integral.core.IntegralClass.get_instrument_band_map')
    def test_get_spectra(self, instrument_band_mock, servlet_mock, epoch_mock, download_mock):
        instrument_band_mock.return_value = mocks.get_instrument_bands()
        servlet_mock.return_value = mocks.get_mock_spectra()
        epoch_mock.return_value = {'epoch': ['today']}
        download_mock.return_value = data_path('tar_file.tar')
    
        isla = IntegralClass()
        mock_instrument_bands(isla_module=isla)
        spectra_list_extracted = isla.get_spectra(target_name='target',
                                                  epoch='today', band='b1')
>       assert len(spectra_list_extracted) == 3
E       TypeError: object of type 'NoneType' has no len()

astroquery/esa/integral/tests/test_isla_tap.py:567: TypeError
___________________________________________ TestIntegralTap.test_get_mosaic ____________________________________________

self = <astroquery.esa.integral.tests.test_isla_tap.TestIntegralTap object at 0x13ba30c20>
instrument_band_mock = <MagicMock name='get_instrument_band_map' id='5302607056'>
servlet_mock = <MagicMock name='execute_servlet_request' id='5302734240'>
epoch_mock = <MagicMock name='get_epochs' id='5302740192'>
download_mock = <MagicMock name='download_file' id='5302687872'>

    @patch('astroquery.esa.integral.core.pyvo.dal.TAPService.capabilities', [])
    @patch('astroquery.esa.utils.utils.download_file')
    @patch('astroquery.esa.integral.core.IntegralClass.get_epochs')
    @patch('astroquery.esa.utils.utils.execute_servlet_request')
    @patch('astroquery.esa.integral.core.IntegralClass.get_instrument_band_map')
    def test_get_mosaic(self, instrument_band_mock, servlet_mock, epoch_mock, download_mock):
        instrument_band_mock.return_value = mocks.get_instrument_bands()
        servlet_mock.return_value = mocks.get_mock_mosaic()
        epoch_mock.return_value = {'epoch': ['today']}
        download_mock.return_value = data_path('tar_gz_file.gz')
    
        isla = IntegralClass()
        mock_instrument_bands(isla_module=isla)
        mosaics_extracted = isla.get_mosaic(epoch='today', instrument='i1')
>       assert len(mosaics_extracted) == 2
E       TypeError: object of type 'NoneType' has no len()

astroquery/esa/integral/tests/test_isla_tap.py:622: TypeError
=============================================== short test summary info ================================================
SKIPPED [12] ../../../.pyenv/versions/3.12.1/lib/python3.12/site-packages/pytest_remotedata/plugin.py:81: need --remote-data option to run
SKIPPED [1] ../../../.pyenv/versions/3.12.1/lib/python3.12/site-packages/_pytest/doctest.py:458: all tests skipped by +SKIP option
FAILED astroquery/esa/integral/tests/test_isla_tap.py::TestIntegralTap::test_get_short_term_timeseries - TypeError: object of type 'NoneType' has no len()
FAILED astroquery/esa/integral/tests/test_isla_tap.py::TestIntegralTap::test_get_spectra - TypeError: object of type 'NoneType' has no len()
FAILED astroquery/esa/integral/tests/test_isla_tap.py::TestIntegralTap::test_get_mosaic - TypeError: object of type 'NoneType' has no len()
======================================= 3 failed, 34 passed, 13 skipped in 1.05s =======================================

bsipocz · 2025-01-23T04:33:25Z

As for the release: we're doing a quick turnaround for 0.4.9, the plan is to do it this week, but I can certainly wait for this PR to get merged.

jespinosaar · 2025-01-23T09:16:14Z

Hi @bsipocz , I have modified the tests so now the downloaded files should be stored in a temporary path that is cleaned after the tests. So the warnings should have gone.

I am curious about the failing tests. These errors appeared to me when the dummy files were not included yet. But now they should be there. The tests are working in the CI/CD environment and in my local as well (using python setup.py test and tox to execute them).

Maybe you can try executing them again. But, in any case, could you please let me know which command you are using to execute them in your local? So I can trace the issue in my local. Many thanks!

jespinosaar · 2025-01-23T09:16:37Z

As for the release: we're doing a quick turnaround for 0.4.9, the plan is to do it this week, but I can certainly wait for this PR to get merged.

That is great! thanks for letting me know and waiting for this PR!

bsipocz · 2025-01-23T19:40:46Z

I do use pytest -P esa.integral and pytest -P esa.integral -R, but was also running the same tox command CI runs and see the failures on python 3.12, but not with python 3.11.

bsipocz · 2025-01-23T19:53:34Z

I have the strong suspicion this is about the tarfile extractions and that we need to add the fully trusted filter option.

[edit:] yes, it was a tarfile related issue. I'm pushing changes to this PR to get it fixed, but first also changes to the CI so we see the issue in CI, too.

… it triggers the issue we see locally

…ecations

bsipocz · 2025-01-23T23:19:05Z

OK, local test failures are fixed. I suspect some of the issues only shown locally when the tests were run for only one module (some of the settings/etc are reused in the same test run).

Anyway, the one remaining issue is that there are still file dumps done with the esa.utils tests.

bsipocz · 2025-01-23T23:56:48Z

These are the still dumped files:

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	astroquery/esa/utils/tests/data/tar_file_250123155443/
	astroquery/esa/utils/tests/data/tar_gz_file_250123155443/

However, I would say this is a small enough issue with the PR that if I get to the point being close to a release, I may just merge the PR as is and leave this issue for a follow-up cleanup as I don't expect much changes is needed to the API.

jespinosaar · 2025-01-24T07:28:52Z

I have the strong suspicion this is about the tarfile extractions and that we need to add the fully trusted filter option.

[edit:] yes, it was a tarfile related issue. I'm pushing changes to this PR to get it fixed, but first also changes to the CI so we see the issue in CI, too.

Many thanks for finding this! This will improve the CI workflow for sure

jespinosaar · 2025-01-24T07:29:38Z

These are the still dumped files:
Untracked files:
  (use "git add <file>..." to include in what will be committed)
	astroquery/esa/utils/tests/data/tar_file_250123155443/
	astroquery/esa/utils/tests/data/tar_gz_file_250123155443/
However, I would say this is a small enough issue with the PR that if I get to the point being close to a release, I may just merge the PR as is and leave this issue for a follow-up cleanup as I don't expect much changes is needed to the API.

I just pushed some small changes, so now the uncompressed files are copied to a temp folder before and they are removed after the tests. So no untracked files should appear now.

Please, let me know if you still see them and more changes are required.

bsipocz

Thank you. It all looks good now.

…query ESA Integral Science Legacy Archive Astroquery module

jespinosaar · 2025-01-27T09:35:05Z

Many thanks for your support, @bsipocz !!

jespinosaar self-assigned this Dec 18, 2024

jespinosaar added the esa label Dec 18, 2024

bsipocz added New Service esa.integral labels Dec 18, 2024

jespinosaar force-pushed the ESA_isla-create-astroquery branch from e9d45f3 to 8368fd6 Compare December 19, 2024 08:41

bsipocz added this to the v0.4.9 milestone Dec 19, 2024

jespinosaar added 14 commits January 7, 2025 15:04

INTEGRALSWRQ-154: initial version of astroquery module for ISLA

6d895fe

INTEGRALSWRQ-154: download methods and documentation updated

ce4aef9

INTEGRALSWRQ-154: utils refactored

789463f

INTEGRALSWRQ-158: login and logout included

d75b02f

INTEGRALSWRQ-158: tests updated, target search with resolver

16f869c

INTEGRALSWRQ-158

d546c1e

INTEGRALSWRQ-158: tests and documentation updated

14add85

INTEGRALSWRQ-158: pycodestyle fixes

2271264

INTEGRALSWRQ-158: pycodestyle fixes 2

6791e97

INTEGRALSWRQ-158: pycodestyle fixes 3

77bf95a

INTEGRALSWRQ-158: changelog added

3baa435

INTEGRALSWRQ-158: url updated

d4e38f9

INTEGRALSWRQ-158: tests fixed

e77643e

INTEGRALSWRQ-158: pycodestyle fixes 4

dc9f7c8

jespinosaar force-pushed the ESA_isla-create-astroquery branch from af9d8e6 to dc9f7c8 Compare January 7, 2025 14:10

INTEGRALSWRQ-158: minor fix in documentation

ed9348f

bsipocz reviewed Jan 17, 2025

View reviewed changes

jespinosaar added 3 commits January 21, 2025 15:38

INTEGRALSWRQ-158: download methods replaced by get only

0bc450b

INTEGRALSWRQ-158: removed unused imports

a91ffca

INTEGRALSWRQ-158: get_timeline with skycoords

1cfe16a

jespinosaar added 3 commits January 22, 2025 13:20

INTEGRALSWRQ-158: remote tests for integral

651d103

INTEGRALSWRQ-158: codestyle after remote tests for integral

089e777

INTEGRALSWRQ-158: tests for utils

1a58a5c

EHSTSWRQ-54: tests with temporary paths

77dcba4

bsipocz added 3 commits January 23, 2025 12:12

CI: adding new CI job, and doing it as part of unrelated PR to see if…

571e133

… it triggers the issue we see locally

MAINT: patching tarfile as we trust ESA files, and need to avoid depr…

98945db

…ecations

FIX: use temp directory for download testing

caa8a7c

jespinosaar added 2 commits January 24, 2025 08:20

INTEGRALSWRQ-158: removing temp files for esa.utils tests

5050e65

INTEGRALSWRQ-158: pycodestyle fixes in esa.utils

f181ac8

bsipocz approved these changes Jan 24, 2025

View reviewed changes

bsipocz merged commit a1c1156 into astropy:main Jan 24, 2025
12 checks passed

bsipocz added a commit that referenced this pull request Jan 24, 2025

Merge pull request #3154 from esdc-esac-esa-int/ESA_isla-create-astro…

27b254d

…query ESA Integral Science Legacy Archive Astroquery module

		return response


		def plot_result(x, y, x_title, y_title, plot_title, *, error_x=None, error_y=None, log_scale=False):

		return super()._request(method, url, **kwargs)


		def get_coord_input(value, msg):

ESA Integral Science Legacy Archive Astroquery module #3154

ESA Integral Science Legacy Archive Astroquery module #3154

Conversation

jespinosaar commented Dec 18, 2024 • edited Loading

pep8speaks commented Dec 18, 2024 • edited Loading

Comment last updated at 2025-01-22 12:29:15 UTC

jespinosaar commented Dec 18, 2024

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

jespinosaar commented Dec 19, 2024

bsipocz commented Dec 19, 2024

jespinosaar commented Dec 19, 2024

jespinosaar commented Jan 7, 2025

bsipocz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jespinosaar commented Jan 17, 2025

jespinosaar commented Jan 21, 2025

jespinosaar commented Jan 22, 2025 • edited Loading

bsipocz commented Jan 23, 2025

bsipocz commented Jan 23, 2025

jespinosaar commented Jan 23, 2025

jespinosaar commented Jan 23, 2025

bsipocz commented Jan 23, 2025

bsipocz commented Jan 23, 2025 • edited Loading

bsipocz commented Jan 23, 2025

bsipocz commented Jan 23, 2025

jespinosaar commented Jan 24, 2025

jespinosaar commented Jan 24, 2025 • edited Loading

bsipocz left a comment

Choose a reason for hiding this comment

jespinosaar commented Jan 27, 2025

jespinosaar commented Dec 18, 2024 •

edited

Loading

pep8speaks commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

jespinosaar commented Jan 22, 2025 •

edited

Loading

bsipocz commented Jan 23, 2025 •

edited

Loading

jespinosaar commented Jan 24, 2025 •

edited

Loading