Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests to i.modis, i.landsat, i.sentinel download #1033

Open
3 tasks
lucadelu opened this issue Mar 3, 2024 · 23 comments
Open
3 tasks

Add tests to i.modis, i.landsat, i.sentinel download #1033

lucadelu opened this issue Mar 3, 2024 · 23 comments
Labels
enhancement New feature or request Python Related code is in Python

Comments

@lucadelu
Copy link
Contributor

lucadelu commented Mar 3, 2024

Add test to the following modules:

  • i.landsat.download
  • i.modis.download
  • i.sentinel.download
@lucadelu lucadelu added enhancement New feature or request Python Related code is in Python labels Mar 3, 2024
@HamedElgizery
Copy link
Contributor

Hello, I am looking forward to contributing to "Add EODAG support to GRASS GIS" for GSoC.

Is anyone working on this issue? If not, I will be adding tests for them.

@echoix
Copy link
Member

echoix commented Mar 19, 2024

@HamedElgizery The two thumbs most probably mean you're good to go. Otherwise you'd might only get an answer during the weekend. We'll be happy to help you when reviewing ;)

@ninsbl
Copy link
Member

ninsbl commented Mar 19, 2024

Nice.

See also OSGeo/grass#3322 (comment) regarding download tests.

There may also be a basic question of using unit tests or pytest. Beyond GRASS unit tests for other modules, Pytests for e.g. sentinelsat (https://github.com/sentinelsat/sentinelsat/tree/main/tests) may be worth having a look at...

@HamedElgizery
Copy link
Contributor

HamedElgizery commented Mar 20, 2024

@ninsbl
Thank you for the references!
I have looked through them. I believe gunittest would be the most convenient, so that we can utilize its custom features. Do you think that isn't necessarily the case?

Also, I have been looking through i.sentinel.download, and I was trying to run it, but it seems like I have to fully compile GRASS to be able to run the addons, is there a way to work with the addons modules without having to compile the GRASS core source code?

@ninsbl
Copy link
Member

ninsbl commented Mar 20, 2024

i.sentinel.download is a python Addon, so you do not have to compile it. You can run it e.g. from the source directory with:
python3 src/imagery/i.sentinel/i.sentinel.download/i.download.py when you are in a GRASS session... and you can use GRASS installed from your package manager...

@lucadelu
Copy link
Contributor Author

Nice.

See also OSGeo/grass#3322 (comment) regarding download tests.

There may also be a basic question of using unit tests or pytest. Beyond GRASS unit tests for other modules, Pytests for e.g. sentinelsat (https://github.com/sentinelsat/sentinelsat/tree/main/tests) may be worth having a look at...

According to @marisn comment seems not a good idea to test the download, but how can we check these modules?

Another topic is how to manage user and password for the different services, is there a suggested way to do that?

@marisn
Copy link
Contributor

marisn commented Mar 21, 2024

According to @marisn comment seems not a good idea to test the download, but how can we check these modules?

Tests should not depend on external systems unless you test those external systems. If network connection or remote server goes down, it will cause test failure although the code in question is just fine.

I haven't looked into the code, but generally there are two ways of action:

  • If separate Python functions can be tested, just mock requests library and make it return expected and unexpected data.
  • If it is not possible to test separate functions, you'll have to create a minimalistic server implementation and test against it.
  • If neither of options are possible to implement, the code to evaluate needs to be changed to make it testable. (e.g. break up code into separate functions that can be tested separately or make possible to inject URLs for testing)

Connecting to external system during a test is fine iff you test if URLs/API of external system haven't changed. Still then failures of tests should not count against our code and should just generate a warning (as the issue might be just of temporary nature).

@ninsbl
Copy link
Member

ninsbl commented Mar 21, 2024

Another topic is how to manage user and password for the different services, is there a suggested way to do that?

In principle, credentials can be stored in GitHub secrets and made available as environment variables in the workflow. However, some systems (like ASF) require users to rotate their password, unfortunately without an option to do so programatically. So that would be a maintenance issue. However, with @marisn `s suggestions connecting to external systems might not be necessary at all....

@ninsbl
Copy link
Member

ninsbl commented Mar 21, 2024

Given @marsin `s useful comments, I would suggest to add a "p-flag" to i.sentinel.download, that returns the actual query string that would be send to the DHUS repository.

Here is an example that should work:

import sentinelsat
SAPI = sentinelsat.SentinelAPI("test", "test", api_url="test")
SAPI.format_query(ingestion_date=("2023-01-10", "2023-01-12"))

This should return:

 'ingestion_date:["2023-01-10" TO "2023-01-12"]'

That way users can inspect a query before it is sent and the result can be used in classic unit tests.

Other stuff could be tested in doc ests I guess...?

Anyway, for the GSoC skills test, I would suggest writing a set of unit tests for the query compiled by sentinelsat should be sufficient, no?

Not sure about the landsatxplore API...

P.S.: I am about to send a PR for some minor issues with i.sentinel.download, I may add a p-flag in that context...
Will have a look at it...

@veroandreo
Copy link
Contributor

Not sure about the landsatxplore API...

landsatxplore API is no longer maintained by its developer unfortunately, so the use of something else in i.landsat.download is kinda urgent to ensure the extension is functional again.

Both Markus and I were granted admin rights in landsatxplore repo, but it's pretty difficult to keep up.

@ninsbl
Copy link
Member

ninsbl commented Mar 21, 2024

See: #1044 for i.sentinel.download

@marisn
Copy link
Contributor

marisn commented Mar 22, 2024

I peaked into the i.sentinel.download code – it seemed to be well split into functions and thus it should be easy¹ to test those functions separately. Python mock library is great for uncoupling functions from the rest of code. E.g. download_gcs_file function can be imported into test file, requests.get can be mocked to do whatever is required – to return an error, return incorrect data or return a file with correct checksum. Thus no requests would be sent to an external system and all code paths can be tested.

¹ for certain meanings of word "easy".

@veroandreo
Copy link
Contributor

Since we have limited time (deadline is April 2nd) what about writing tests for any of the imagery modules that are missing them? See for example i.landsat.toar or i.pca.

@HamedElgizery
Copy link
Contributor

HamedElgizery commented Mar 23, 2024

See for example i.landsat.toar or i.pca.

Is there a document, or a research paper by chance, that describes how i.pca works?
I am trying to figure out what is the responsibility of each function in i.pca, and what would be the expected output.

I am also thinking of using a standard input -- maybe the ones used in the HTML document -- and save the inputs and the outputs for each function into the tests for now...

@echoix
Copy link
Member

echoix commented Mar 23, 2024

Is the documentation linked to in https://grass.osgeo.org/grass-devel/manuals/i.pca.html enough? There is a paper, and another article on the wiki https://grasswiki.osgeo.org/wiki/Principal_Components_Analysis, that is quite filled, with even more info.

PCA is also common in machine learning/Artificial intelligence when doing some feature engineering, it is one of the old math tricks available. Since it is a hot topic now, you'd be able to find content on this everywhere outside of the GRASS context.

@HamedElgizery
Copy link
Contributor

Sounds good, thanks!
I will learn more about it.

@echoix
Copy link
Member

echoix commented Mar 23, 2024

That too is nice and easy to read to understand the concept https://medium.com/@dareyadewumi650/understanding-the-role-of-eigenvectors-and-eigenvalues-in-pca-dimensionality-reduction-10186dad0c5c

It's pure maths (or engineering maths too).

@lucadelu
Copy link
Contributor Author

@HamedElgizery do you need any help?

@HamedElgizery
Copy link
Contributor

HamedElgizery commented Mar 31, 2024

Sorry for the late reply in this issue, @lucadelu .
I have been working on the document I emailed you and here is the first draft: https://docs.google.com/document/d/1PlUhGX_OWAOkdkHmH5smiYEgAl0yvqPh8tAa14WbE2E/edit?usp=sharing

I have read about PCA and I will be working on writing an initial test for it. It is still a bit confusing to me how the input is formatted in the i.pca module, though.

@veroandreo
Copy link
Contributor

@HamedElgizery remember to introduce yourself to the extended GRASS community via the grass-dev mailing list and share your proposal there as other candidates have done so far, see eg: https://www.mail-archive.com/[email protected]/msg61769.html.

@HamedElgizery
Copy link
Contributor

I have gone through the suggestions and added them accordingly. I also sent an email to introduce myself, and I have shared the draft through the mailing list. 🙏

@ninsbl
Copy link
Member

ninsbl commented Mar 31, 2024

I have read about PCA and I will be working on writing an initial test for it. It is still a bit confusing to me how the input is formatted in the i.pca module, though.

Hi @HamedElgizery and welcome. The simplest way to add a test to i.pca is to:

  1. get the sample data that is used in CI, e.g. with: g.download.location url=https://grass.osgeo.org/sampledata/north_carolina/nc_spm_full_v2alpha2.tar.gz path=$HOME
  2. use an existing unit test as a starting point, e.g.: https://github.com/OSGeo/grass/blob/main/imagery/i.vi/testsuite/test_vi.py
  3. modify it e.g. using the example from the i.pca manual. Here yo have to get the refence values from the results of the example computation, assuming the current module implementation delivers correct results....

You may modify other aspects of the i.vi-test, like using assertRasterFitsUnivar (https://grass.osgeo.org/grass83/manuals/libpython/gunittest.html#gunittest.case.TestCase.assertRasterFitsUnivar) instead of assertRasterMinMax or implement other improvements you may see fit in an i.pca test...

Good luck and let us know if you have any more specific questions while working on the test...

@HamedElgizery
Copy link
Contributor

@ninsbl Thank you for the guidance it helped me get started!
I created this pull request: OSGeo/grass#3550
Can you please review it and let me know what you think :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Python Related code is in Python
Projects
None yet
Development

No branches or pull requests

6 participants