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

calculate indices in client using one-liner #255

Closed
wants to merge 0 commits into from

Conversation

Bart92
Copy link
Contributor

@Bart92 Bart92 commented Nov 3, 2021

Die testen moet ik echt nog aanpassen, maar ik weet niet zo goed hoe.
Ik heb gemerkt dat met die equals in de test_get_expression_map ik eender welke waarde in kan geven voor array_create en in alle gevallen gaat de test groen markeren, wat te maken zou moeten hebben met de implementatie van de eq (die eq aanroept, die er in de client ok uitziet).
Heb jij nog ideeën hoe ik die testen zou kunnen verbeteren?

@Bart92 Bart92 requested a review from soxofaan November 3, 2021 10:57
Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first round of review notes 😅

openeo/extra/spectral_indices/__init__.py Outdated Show resolved Hide resolved
openeo/extra/spectral_indices/spectral_indices.py Outdated Show resolved Hide resolved
openeo/extra/spectral_indices/spectral_indices.py Outdated Show resolved Hide resolved
openeo/extra/spectral_indices/spectral_indices.py Outdated Show resolved Hide resolved
openeo/extra/spectral_indices/spectral_indices.py Outdated Show resolved Hide resolved
openeo/extra/spectral_indices/spectral_indices.py Outdated Show resolved Hide resolved
tests/test_spectral_indices.py Outdated Show resolved Hide resolved
tests/test_spectral_indices.py Outdated Show resolved Hide resolved
tests/test_spectral_indices.py Outdated Show resolved Hide resolved
tests/test_spectral_indices.py Outdated Show resolved Hide resolved
@m-mohr
Copy link
Member

m-mohr commented Nov 3, 2021

I found this via #246 as I'm interested in how this aligns with OGC CQL and JS, but I can't understand what it's about. Could you translate it, please? ;-)

@soxofaan
Copy link
Member

soxofaan commented Nov 3, 2021

Could you translate it, please? ;-)

The original post is not really explaining the PR, it's mainly wondering about how to write the unit tests.

But in a nutshell, this PR adds a helper module to easily calculate the spectral indices defined in https://github.com/davemlz/eemont/blob/master/eemont/data/spectral-indices-dict.json:
user specifies that they want for example index "ARVI",
the python client looks up the formula for that :

"formula": "(N - (R - gamma * (R - B))) / (N + (R - gamma * (R - B)))",

and translates that into a process graph (based on band name mapping)

@soxofaan
Copy link
Member

soxofaan commented Nov 3, 2021

maybe this is a better reference to this spectral indices repo: https://github.com/davemlz/awesome-spectral-indices

@m-mohr
Copy link
Member

m-mohr commented Nov 3, 2021

Thanks, this is interesting. It seems that this is not a general "band math parser", right? Or I have missed the parsing in the code...

@soxofaan
Copy link
Member

soxofaan commented Nov 3, 2021

It seems that this is not a general "band math parser", right? Or I have missed the parsing in the code...

current implementation does not really define a parser, but uses an eval() trick (so assumes the formula is valid Python)

openeo/extra/spectral_indices/spectral_indices.py Outdated Show resolved Hide resolved
tests/test_spectral_indices.py Outdated Show resolved Hide resolved
tests/test_spectral_indices.py Outdated Show resolved Hide resolved
tests/test_spectral_indices.py Outdated Show resolved Hide resolved
"reference": "",
"short_name": "ANIR",
"type": "vegetation",
"range": "(0,1)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is not a direct copy from upstream, we should document (e.g. in a README) how this file was generated/edited, so that we can properly adapt it when there are "upstream" updates.
Even better would be a script that automatically generates an adapted file from the upstream source.

openeo/extra/spectral_indices/spectral_indices.py Outdated Show resolved Hide resolved
soxofaan added a commit that referenced this pull request Nov 4, 2021
return array_modify(data=x_res, values=index_values, index=len(datacube.metadata.band_names))


def compute_indices(datacube: DataCube, index_list: list, uplim_rescale: int = None) -> DataCube:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this compute_indices works with apply_dimension where multiple indices are appended to the cube

I think user would also expect other approaches:

  • compute multiple indices but disregard all original bands
  • compute single index with reduce_dimension (e.g. how all the original NDVI examples are done)

soxofaan added a commit that referenced this pull request Nov 9, 2021
no need to throw exception when collection band is "unknown" (it's not going to be used anyway)
soxofaan added a commit that referenced this pull request Nov 10, 2021
soxofaan added a commit that referenced this pull request Nov 10, 2021
soxofaan added a commit that referenced this pull request Nov 16, 2021
soxofaan added a commit that referenced this pull request Nov 16, 2021
soxofaan added a commit that referenced this pull request Nov 16, 2021
soxofaan added a commit that referenced this pull request Nov 16, 2021
soxofaan added a commit that referenced this pull request Nov 16, 2021
no need to throw exception when collection band is "unknown" (it's not going to be used anyway)
soxofaan added a commit that referenced this pull request Nov 16, 2021
soxofaan added a commit that referenced this pull request Nov 16, 2021
soxofaan added a commit that referenced this pull request Nov 16, 2021
soxofaan added a commit that referenced this pull request Nov 16, 2021
@soxofaan soxofaan closed this Nov 16, 2021
@soxofaan soxofaan force-pushed the EP4053-compute-indices branch from 31b9c92 to 9bb461d Compare November 16, 2021 13:55
@soxofaan soxofaan deleted the EP4053-compute-indices branch November 16, 2021 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants