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

Refactor all indices to one module. #285

Merged
merged 15 commits into from
Jan 1, 2022

Conversation

RitwikGupta
Copy link
Collaborator

All the indices currently included are just ratios of sums. The helper functions also depend on the user knowing what the index of the relevant bands are to begin with.

Since this knowledge of the underlying index is already required, it's cleaner to have one generalized module for difference indices.

@adamjstewart
Copy link
Collaborator

This won't work in general. Although all of our current indices are difference ratios, most indices are not: https://www.indexdatabase.de/db/i.php

We could have a single difference ratio index to represent all, but I'm not sure how much of an advantage that will give us. @isaaccorley thoughts?

@adamjstewart adamjstewart added the transforms Data augmentation transforms label Dec 15, 2021
@adamjstewart adamjstewart added this to the 0.2.0 milestone Dec 15, 2021
@RitwikGupta
Copy link
Collaborator Author

That is my thought; one module for all difference indices and then we can add more specific ones as needed. Keeps it clean.

@isaaccorley
Copy link
Collaborator

isaaccorley commented Dec 15, 2021

This won't work in general because our current indices are all normalized difference indices but many indices have more than 2 bands, can be normalized in different ways, or some indices can be a function of other indices. The other reasoning for keeping a separate class per index is that we were including the correct DOI for each index. There seems to some confusion in the community on the original publication source of certain spectral indices and this would help these papers be cited properly.

The refactor plan was to break out the Append into it's own transform that can be used like Append(NDVI(...), dim=1). The other benefit of keeping the dimension as an argument is you can modify it to work as a transform in a dataset, a transform which operates on batches, or for time-series sample/batch.

Edit: although I do think the tests could be refactored some as they are repetitive.

@calebrob6
Copy link
Member

our current indices are all normalized difference indices

I'm not sure I understand the significance of the "normalized" part. These indices are all just a function of some number of bands. In fact, our indices modules could take advantage of the fact that the RasterDatasets know which bands are which.

The other reasoning for keeping a separate class per index is that we were including the correct DOI for each index.

I think we can keep the DOIs around easily by making say AppendNDBI inherit from what @RitwikGupta is proposing (AppendIndex or a AppendDifferenceIndex).

We should definitely have https://www.indexdatabase.de/db/i.php in the documentation.

@RitwikGupta
Copy link
Collaborator Author

👆 I am still unfamiliar with the TorchGeo API (working on it!), but my suggestion was going to be to used NamedTensors (or the RasterDataset implementation equivalent) to make a difference index factory, essentially.

@adamjstewart
Copy link
Collaborator

No worries, our API is constantly evolving anyway, thanks for all the fresh ideas!

@calebrob6
Copy link
Member

Okay, so how does everyone feel about a parent AppendDifferenceIndex with 4 (5 including your other PR) children? This should definitely remove some code, keep the DOIs around (I agree with Isaac that this is important!), and make the path to adding other classes of indices more straightforward.

@RitwikGupta
Copy link
Collaborator Author

Good by me. So that looks like a super + subclasses? __init__() can hold the properly named kwargs and we can pass them to super().__init__(band_a, band_b)

@adamjstewart
Copy link
Collaborator

AppendDifferenceIndex

I would call it AppendNormalizedDifferenceIndex. If it was just a difference, it would look like (x - y). This is a normalized difference since we also divide by the sum.

@github-actions github-actions bot added the testing Continuous integration testing label Jan 1, 2022
torchgeo/transforms/indices.py Show resolved Hide resolved
torchgeo/transforms/indices.py Outdated Show resolved Hide resolved
adamjstewart
adamjstewart previously approved these changes Jan 1, 2022
@calebrob6 calebrob6 enabled auto-merge (squash) January 1, 2022 19:54
@calebrob6 calebrob6 merged commit 3514724 into microsoft:main Jan 1, 2022
@adamjstewart
Copy link
Collaborator

@isaaccorley our "Indices" tutorial still uses torchgeo.transforms.ndvi which was removed in this PR. Can you modify the tutorial and submit a PR against the releases/v0.2 branch?

@adamjstewart adamjstewart added utilities Utilities for working with geospatial data and removed utilities Utilities for working with geospatial data labels Jan 2, 2022
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* Add compute index helper function + refactor

* Generalize all indices to one function

* Refactor indices to one module

* Fix init imports

* Add docstrings

* Fix transform test

* update to inherit from parent index class

* add nbr index

* forgot self in method

* mypy and dim fixes

* update expected tensor in test

* Update indices.py

* Black

* Update indices.py

* Black again

Co-authored-by: isaaccorley <22203655+isaaccorley@users.noreply.github.com>
Co-authored-by: Caleb Robinson <calebrob6@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Continuous integration testing transforms Data augmentation transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants