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

Indices Transforms #127

Merged
merged 40 commits into from Sep 28, 2021
Merged

Conversation

isaaccorley
Copy link
Collaborator

@isaaccorley isaaccorley commented Sep 11, 2021

This is simply a draft of a transform for computing NDVI and concatenating to the sample["image"] channels. Open to suggestions/feedback.

Note: I just realized that the requiring the index of the channels for computing the indices doesn't scale. See Enhanced Vegation Index which requires 4+ bands to compute EVI = G * ((NIR - R) / (NIR + C1 * R – C2 * B + L)).

Update:

This PR adds the following:

  • AugmentationSequential wrapper around kornia.augmentation.AugmentationSequential which supports our sample/batch dicts.
  • ndbi, ndsi, ndvi, ndwi functionals which given specific multispectral band input will compute the associated index.
    AppendNDBI, AppendNDSI, AppendNDVI, AppendNDWI transform modules which take as input a batch dict, computes the desired index, and appends to the channel dimension.
  • Unit tests
  • kornia>=0.5.4 as dependency which is the version when kornia.augmentation.AugmentationSequential was added. Additionally, kornia's only dependency is pytorch.

TODO:

  • Add jupyter notebook to docs/tutorials display the usage of indices transforms
  • Fix notebook sphinx errors

Closes #112

Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

This might need a bit more thought before we can safely integrate this with Kornia.

torchgeo/transforms/indices.py Show resolved Hide resolved
torchgeo/transforms/indices.py Outdated Show resolved Hide resolved
torchgeo/transforms/indices.py Outdated Show resolved Hide resolved
torchgeo/transforms/indices.py Show resolved Hide resolved
@adamjstewart adamjstewart added the transforms Data augmentation transforms label Sep 12, 2021
@isaaccorley isaaccorley marked this pull request as draft September 20, 2021 04:13
@isaaccorley
Copy link
Collaborator Author

isaaccorley commented Sep 20, 2021

@adamjstewart @calebrob6 I'm curious if we should even utilize kornia to facilitate augmentations when these are instead simply transforms. We could use it along with kornia and the AugmentationSequential wrapper I created like below:

import torch
import torch.nn as nn
import kornia.augmentation as K
from torchgeo.transforms import AugmentationSequential, AppendNDVI, AppendNDBI

sample = dict(
   image=torch.randn(2, 9, 512, 512).to("cuda"),
   mask=torch.randint(0, 10, size=(2, 1, 512, 512)).to(torch.float).to("cuda") # kornia requires mask to be float for some reason
)
transforms = nn.Sequential(
   AppendNDVI(index_red=2, index_nir=3),
   AppendNDBI(index_swir=4, index_nir=3),
   AugmentationSequential(
      K.RandomHorizontalFlip(),
      K.RandomVerticalFlip(),
      data_keys=["image", "mask"]
   )   
)
transforms = transforms.to("cuda")
outputs = transforms(sample)
# "image": (2, 11, 512, 512)
# "mask": (2, 1, 512, 512)

The above actually works as of now and simply chains multiple non augmentation transforms (e.g. computing indices), along with an AugmentationSequential wrapper which then performs the transforms on the image and mask.

@calebrob6
Copy link
Member

calebrob6 commented Sep 20, 2021

The above seems fine to me! Thoughts:

  • This seems remote sensing specific and doesn't need to be added to kornia
  • Things we write here should be explicit about whether they work on batches or samples (or both!). From the limited amount I've seen, the kornia augmentation pipelines operate on batches, while torchvision transforms operate on samples.

The name of the game is to be as friendly to existing CV code as possible.

@isaaccorley
Copy link
Collaborator Author

That's a great point about sample vs batch. Apparently torchvision transforms on tensors work for both batch and sample data. Although, under the hood, I believe they just add a batch dim of 1 to sample data and then squeeze it off afterwards.

A note on the AugmentationSequential wrapper. I'm actually hoping that a PR I'm working on for adding this to kornia will be accepted. Until then, using the wrapper should allow us to chain transforms and augs similarly to the above example.

@isaaccorley
Copy link
Collaborator Author

I'm running into a very strange mypy error due to something in kornia. Tracking it down...

mypy .
Traceback (most recent call last):
  File "/home/icorley/miniconda3/envs/torchgeo/bin/mypy", line 8, in <module>
    sys.exit(console_entry())
  File "/home/icorley/miniconda3/envs/torchgeo/lib/python3.9/site-packages/mypy/__main__.py", line 11, in console_entry
    main(None, sys.stdout, sys.stderr)
  File "mypy/main.py", line 87, in main
  File "mypy/main.py", line 165, in run_build
  File "mypy/build.py", line 179, in build
  File "mypy/build.py", line 254, in _build
  File "mypy/build.py", line 2697, in dispatch
  File "mypy/build.py", line 3021, in process_graph
  File "mypy/build.py", line 3138, in process_stale_scc
  File "mypy/build.py", line 2288, in write_cache
  File "mypy/build.py", line 1475, in write_cache
  File "mypy/nodes.py", line 313, in serialize
  File "mypy/nodes.py", line 3149, in serialize
  File "mypy/nodes.py", line 3083, in serialize
AssertionError: Definition of kornia.geometry.subpix.spatial_soft_argmax.normalize_pixel_coordinates is unexpectedly incomplete

@calebrob6
Copy link
Member

I'm running into a very strange mypy error due to something in kornia. Tracking it down...

I'm getting this same error in the trainers PR

@calebrob6
Copy link
Member

spatial_soft_argmax.py needs to do from kornia.geometry.conversions import normalize_pixel_coordinates, normalize_pixel_coordinates3d instead of from kornia.geometry...

@isaaccorley
Copy link
Collaborator Author

isaaccorley commented Sep 23, 2021

Ok, I'll make an issue in the kornia repo.

Edit: see kornia/kornia#1318

@isaaccorley isaaccorley reopened this Sep 24, 2021
@isaaccorley
Copy link
Collaborator Author

Sphinx seems to be failing due to warning that it doesn't allow cells with shell commands in them (e.g. !pip install kornia). I could rewrite using Python but I'm curious if we even want this level of strictness?

@adamjstewart
Copy link
Collaborator

Sphinx seems to be failing due to warning that it doesn't allow cells with shell commands in them (e.g. !pip install kornia). I could rewrite using Python but I'm curious if we even want this level of strictness?

That's odd, can you open an issue at https://github.com/spatialaudio/nbsphinx/issues? We definitely want/need this to be supported. This is a very common thing to have in a notebook and is a requirement for our code to work on Colab.

P.S. We need to convert our other notebooks to pip install torchgeo... at some point. Since our GitHub Actions for testing notebooks already runs pip install we don't have to worry about it installing a different version from PyPI. I don't love that it can't be tested locally though. Haven't figured that one out yet.

setup.cfg Outdated Show resolved Hide resolved
@isaaccorley isaaccorley marked this pull request as ready for review September 25, 2021 02:00
Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

Should we get rid of torchgeo/transforms/transforms.py? We're already planning on dropping the random flip transforms, and we could move Identity to the tests themselves. I'm not sure where the best place to put AugmentationSequential would be. It could be in __init__.py itself or in torchgeo/transforms/containers.py.

I still need to look at the tutorial. I still have a lot of questions remaining about how Kornia does things and how TorchGeo is compatible with Kornia, but I'll save those for later.

setup.cfg Outdated Show resolved Hide resolved
.github/workflows/tests.yaml Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
tests/transforms/test_transforms.py Outdated Show resolved Hide resolved
torchgeo/transforms/indices.py Outdated Show resolved Hide resolved
torchgeo/transforms/indices.py Outdated Show resolved Hide resolved
torchgeo/transforms/transforms.py Show resolved Hide resolved
torchgeo/transforms/transforms.py Outdated Show resolved Hide resolved
torchgeo/transforms/transforms.py Outdated Show resolved Hide resolved
@isaaccorley
Copy link
Collaborator Author

isaaccorley commented Sep 25, 2021

Should we get rid of torchgeo/transforms/transforms.py? We're already planning on dropping the random flip transforms, and we could move Identity to the tests themselves. I'm not sure where the best place to put AugmentationSequential would be. It could be in __init__.py itself or in torchgeo/transforms/containers.py.

I still need to look at the tutorial. I still have a lot of questions remaining about how Kornia does things and how TorchGeo is compatible with Kornia, but I'll save those for later.

Great question, if kornia adds support for optionally taking a dict input and returning a dict output to AugmentationSequential where only the specified keys are modified, we could actually drop the AugmentationSequential wrapper entirely and transforms.py as well. Then kornia would only be used in our trainers and tutorials.

They are also working on finishing up a "LambdaAugmentation" class which allows a user to add a general operation to images or masks or both which is simply a nn.Module without having to subclass their AugmentationBase classes and write your own. This would allows us to contain kornia usage only in the trainers but still have a transforms.py of general nn.Modules which modify images using torch ops.

Edit:
Something to point out is that we don't actually need our version of torch.nn.Identity other than to be compatible with type checking since nn.Identity input/output is a Tensor and ours is a dict. Both simply return the input as output.

@isaaccorley
Copy link
Collaborator Author

Sphinx seems to be failing due to warning that it doesn't allow cells with shell commands in them (e.g. !pip install kornia). I could rewrite using Python but I'm curious if we even want this level of strictness?

That's odd, can you open an issue at https://github.com/spatialaudio/nbsphinx/issues? We definitely want/need this to be supported. This is a very common thing to have in a notebook and is a requirement for our code to work on Colab.

P.S. We need to convert our other notebooks to pip install torchgeo... at some point. Since our GitHub Actions for testing notebooks already runs pip install we don't have to worry about it installing a different version from PyPI. I don't love that it can't be tested locally though. Haven't figured that one out yet.

Just to circle back to this. I was able to get around this by using cell magic % instead of exclamation marks ! for executing things like %pip install or directory manipulation (e.g. cd, ls, etc.).

@isaaccorley
Copy link
Collaborator Author

isaaccorley commented Sep 25, 2021

Kornia's AugmentationSequential seems to be broken for bounding boxes. Even when using only AugmentationSequential(K.RandomHorizontalFlip(p=1.0, p_batch=1.0), data_keys=["image", "mask", "boxes"]) I get different values each time I feed our dicts through. Sometimes I get negative transformed bbox values (???).

I'll make an issue on their repo.

Edit: Scratch that. I fixed it.

@adamjstewart
Copy link
Collaborator

Something to point out is that we don't actually need our version of torch.nn.Identity other than to be compatible with type checking since nn.Identity input/output is a Tensor and ours is a dict. Both simply return the input as output.

Didn't realize this already existed, perfect!

@adamjstewart
Copy link
Collaborator

Made the following changes to the notebooks:

  • Moved the tutorials higher up in the list since they will be more commonly used
  • Removed the "Open in Colab" buttons, these are added by nbsphinx at document generation time
  • Switched the pip install to the main repo
  • Download dataset to a temp directory
  • Converted to 4-space indentation
  • Replaced torch.clip with torch.clamp, see Add support for older PyTorch versions #160

Remaining TODOs which can happen in a follow-up PR:

  • Need to switch to the new EuroSat dataset, or use a different dataset with higher res imagery
  • There isn't a lot of "why we are doing this" in the Transforms tutorial. For example, why are we implementing our own MinMaxNormalize? Is there none in Kornia? Is this just for demonstration purposes so users know how to write their own transforms?
  • What's all this stuff?
    #@title EuroSat Multispectral (MS) Browser  { run: "auto", vertical-output: true }
    idx = 9889 #@param {type:"slider", min:0, max:26999, step:1}
    
  • The Sentinel image we plot in the Transform tutorial is very hard to see, I would plot the raw image instead of the 0-1 scaled image, see the plot method in GeoDataset
  • Transform tutorial needs more visualization
  • Indices transform might be a bit too complicated
  • For the indices, you say they can be calculated from the formula below, but there is no formula

The Indices tutorial has a suspiciously greater amount of comments than the Transforms tutorial. You didn't get the Indices tutorial from somewhere else did you?

@isaaccorley
Copy link
Collaborator Author

I added additional comments for clarity, so no, I didn't pull this tutorial from somewhere else.

I would recommend opening the notebook in colab otherwise you can't see the visualization app that uses some of the Colab functionality. The image that's plotted might be dark because it's one of the images of a lake/sea.

I'll refactor to use kornia's minmax normalize. Although, they don't have a stretch/percentile normalize so I'll have to hack something together.

@isaaccorley
Copy link
Collaborator Author

isaaccorley commented Sep 28, 2021

I'll make some of the changes in a bit and the rest can come in a separate PR I think.

@isaaccorley
Copy link
Collaborator Author

Some updates:

  • Need to switch to the new EuroSat dataset, or use a different dataset with higher res imagery
  • Switched to torchgeo.datasets.EuroSAT and which removed a lot of boilerplate code.
  • There isn't a lot of "why we are doing this" in the Transforms tutorial. For example, why are we implementing our own MinMaxNormalize? Is there none in Kornia? Is this just for demonstration purposes so users know how to write their own transforms?
  • I added some more markdown sections to shed light on why. Also added a quick benchmark comparison of using the transforms on cpu/gpu and timed them to show why you should prefer chained transforms in a nn.Sequential that can be pushed to the GPU. There is a kornia.enhance.normalize_min_max but there is no class form of it, and it only takes in a single min/max val instead of a list to min/max normalize per channel. Will make a PR in kornia's repo for this.
  • What's all this stuff?
#@title EuroSat Multispectral (MS) Browser  { run: "auto", vertical-output: true }
idx = 9889 #@param {type:"slider", min:0, max:26999, step:1}
  • Added a comment to relay that this can only be viewed in Colab
  • The Sentinel image we plot in the Transform tutorial is very hard to see, I would plot the raw image instead of the 0-1 scaled image, see the plot method in GeoDataset
  • Changed the default displayed image so it's easier to see. It's also blurry because they are 64x64 images that when displayed are very small so I am interpolating them for better visualization.
  • Transform tutorial needs more visualization
  • Will add this in a separate PR
  • Indices transform might be a bit too complicated
  • I could probably remove the COG windowed download parts but thought that might be helpful.
  • For the indices, you say they can be calculated from the formula below, but there is no formula
  • Added the formulas

@adamjstewart
Copy link
Collaborator

Added a comment to relay that this can only be viewed in Colab

The primary purpose of the tutorials is to serve as documentation, the fact that they can be ran on Colab is less important. If this shows up on Read the Docs and doesn't do anything outside of Colab I think we should just remove it. Aren't there Jupyter Notebook magic things to make this work everywhere?

Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

I'm going to merge this as is. At some point before we release I plan on going back through the tutorials and touching them up and testing them, so if there's anything else needed I can work on them then.

@adamjstewart adamjstewart merged commit c9a788a into microsoft:main Sep 28, 2021
@adamjstewart adamjstewart added this to the 0.1.0 milestone Nov 20, 2021
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* draft indices transform

* import sort

* added AugmentationSequential wrapper for dicts

* updated indices

* fix dim concat bugs

* format

* add kornia dependency

* add augmentationsequenal unit tests

* add augmentationsequential support for boxes and mask dtypes

* add indices tests

* Draft indices tutorial notebook

* move notebook to tutorials folder

* mypy fixes

* fix bug when only image key used in AugmentationSequential

* Created using Colaboratory

* added tutorial to docs

* format

* added kornia master branch dependency'

* refactor notebook to use % cell magic and python to download files

* revert kornia version

* install kornia master branch for mypy checks

* update mypy github action install order

* fix divide by zero error in indices

* Created using Colaboratory

* fix nbsphinx errors

* add TODO to remove kornia in tests action

* format setup.cfg

* minor fixes to indices

* remove unecessary variable

* update mask to cast to original dtype

* removed unused ignore comment

* added gray/rgb/multispectral unit tests

* added tests with boxes

* Created using Colaboratory

* Created using Colaboratory

* fix mypy issues

* updated notebooks in docs

* Updates to tutorials

* Created using Colaboratory

* Created using Colaboratory

Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
transforms Data augmentation transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Transforms to compute popular remote sensing indices
3 participants