euroargodev / argopy

A python library for Argo data beginners and experts
https://argopy.readthedocs.io
European Union Public License 1.2
176 stars 38 forks source link

basic integration of argovis API 2.x #371

Closed bkatiemills closed 1 month ago

bkatiemills commented 1 month ago

Here's the most minimal change to use Argovis' v2.x API to fetch Argo core data.

gmaze commented 1 month ago

hi @bkatiemills thanks for the work, this will be awesome !

closes #246

gmaze commented 1 month ago

Unit tests

With the current design of the tests suite, the argovis data fetcher is not tested using our internal mocked http server, therefore:

the argovis data fetcher won't be covered if the API is not considered alive

This is determined using the argopy.utils.isAPIconnected function like this:

argopy.utils.isAPIconnected(src='argovis', data=True)

This must return True.

The check will make use of the api_server_check dictionary, which needs to be updated for v2

gmaze commented 1 month ago

Running tests locally

I tried to put as much as possible info on https://argopy.readthedocs.io/en/latest/contributing.html

Basically, you should create a local python environnement:

Use the envs_manager script from the ci folder of your argopy fork:

./ci/envs_manager -i argopy-py39-all-pinned

Then, activate the new environment and run tests related to the argovis datafetcher:

conda activate argopy-py39-all-pinned
cd argopy/tests
pytest -ra -v -s test_fetchers_data_argovis.py
gmaze commented 1 month ago

@bkatiemills CI Tests look to be passing OK for float and profile access point

But tests fail for region It is this part that may require some updates:

https://github.com/bkatiemills/argopy/blob/41174a7db49dc48c3207dbf84c1aa08decb9eaef/argopy/data_fetchers/argovis_data.py#L498

note that : https://github.com/bkatiemills/argopy/blob/41174a7db49dc48c3207dbf84c1aa08decb9eaef/argopy/data_fetchers/argovis_data.py#L600 is not used and should be removed (probably some old code not cleaned)

gmaze commented 1 month ago

hi @bkatiemills tell me if you're happy with this, I am, we can merge