Add ChangeDetectionTask

[keves1]

This PR is to add a change detection trainer as mentioned in #2382.

Key points/items to discuss:

• I used the OSCD dataset to test with and modified this dataset to use a temporal dimension.
• With the added temporal dimension, Kornia’s AugmentationSequential doesn’t work, but can be combined with VideoSequential to support the temporal dimension (see Kornia docs). I overrode self.aug in the OSCDDataModule to do this but not sure if this should be incorporated into the BaseDataModule instead.
• VideoSequential adds a temporal dimension to the mask. Not sure if there is a way to avoid this, or if this is desirable, but I added an if statement to the AugmentationSequential wrapper to check for and remove this added dimension.
• The OSCDDataModule applies _RandomNCrop augmentation, but this does not work for time series data. I'm not sure how to modify _RandomNCrop to fix this and would appreciate some help/guidance.
• There are a few tests that I need to still make pass.

cc @robmarkcole

[robmarkcole]

I wonder if we should limit the scope to change between two timesteps and binary change - then we can use binary metrics and provide a template for the plot methods. I say this because this is the most common change detection task by a mile. Might also simplify the augmentations approach? Treating as a video sequence seems overkill. Also I understand there is support for multitemporal coming later.

[adamjstewart]

I wonder if we should limit the scope to change between two timesteps

I'm personally okay with this, although @hfangcat has a recent work using multiple pre-event images that would be nice to support someday (could be a subclass if necessary).

and binary change

Again, this would probably be fine as a starting point, although I would someday like to make all trainers support binary/multiclass/multilabel, e.g., #2219.

provide a template for the plot methods.

Could also do this in the datasets (at least for benchmark NonGeoDatasets). We're also trying to remove explicit plotting in the trainers: #2184

I say this because this is the most common change detection task by a mile.

Agreed.

Might also simplify the augmentations approach? Treating as a video sequence seems overkill.

I actually like the video augmentations, but let me loop in the Kornia folks to get their opinion: @edgarriba @johnnv1

Also I understand there is support for multitemporal coming later.

Correct, see #2382 for the big picture (I think I also sent you a recording of my presented plan).

[adamjstewart]

VideoSequential adds a temporal dimension to the mask. Not sure if there is a way to avoid this

Can you try keepdim=True?

I added an if statement to the AugmentationSequential wrapper to check for and remove this added dimension.

@ashnair1 would this work directly with K.AugmentationSequential now? We are trying to phase out our AugmentationSequential wrapper now that upstream supports (almost?) everything we need.

[keves1]

I will go ahead and make changes for this to be for binary change and two timesteps, sounds like a good starting point.

Can you try keepdim=True?

I tried this and it didn't get rid of the other dimension. I also looked into extra_args but didn't see any options to help with this.

Could also do this in the datasets (at least for benchmark NonGeoDatasets). We're also trying to remove explicit plotting in the trainers

I was going to add plotting in the trainer, but would you rather not then? What would this look like in the dataset?

[robmarkcole]

Perhaps there should even be a base class ChangeDetection and subclasses for BinaryChangeDetection etc?

[adamjstewart]

That's exactly what I'm trying to undo in #2219.

[adamjstewart]

I was going to add plotting in the trainer, but would you rather not then?

We can copy-n-paste the validation_step plotting stuff used by other trainers, but that's probably going to disappear soon (I think we're just waiting on testing in #2184.

What would this look like in the dataset?

See OSCD.plot()

[keves1]

I've updated this to now support only binary change with two timesteps. To get test_weight_file in test_change.py to work with the two images stacked on the channel dimension for Unet, I modified the pytest fixture model() in conftest.py to use timm to create the model instead of torchvision, so that an in_channels parameter can be passed.

I still haven't been able to figure out how to make transforms.transforms._RandomNCrop work with the added temporal dimension. It seems to have something to do with _NCropGenerator not properly handling the temporal dimension but I really don't understand what is going on there.

[keves1]

I'm going to need some help figuring out how to get transforms.transforms._RandomNCrop to work with the added temporal dimension (this is used by the OSCD dataset). I've delved into this a few times but with my lack of familiarity with Kornia I haven't been able to track down the source of the issue. You can see the issue by running tests/trainers/test_change.py::TestChangeDetectionTask::test_trainer.

Also, disregard my earlier comments about Kornia VideoSequential adding a dimension to the mask, this seems to have resolved with the latest Kornia version.

[keves1]

I see that in the automated pytest checks (in tests / minimum) there was a syntax error, but I don't see this issue locally. How do I resolve this?

[adamjstewart]

Couldn't reproduce either, might be a bug specific to older Python versions. Anyway, undid the change on the line it was complaining about, let's see if that fixes it.

[keves1]

I noticed I also need to update tests/datasets/test_oscd.py. And I think we are removing tests/datamodules/test_oscd.py per #978 now that we have a change detection task, right?

Also, how do you usually run prettier locally? I see there is an issue in the prettier check here but it isn't installed in the devcontainer unless I'm mistaken.

[adamjstewart]

I think we are removing tests/datamodules/test_oscd.py per #978 now that we have a change detection task, right?

Correct.

Also, how do you usually run prettier locally? I see there is an issue in the prettier check here but it isn't installed in the devcontainer unless I'm mistaken.

To be honest, I rarely run prettier locally. But here are the docs on how to do it: https://torchgeo.readthedocs.io/en/latest/user/contributing.html#linters

I've also never used the devcontainer before. But it looks like there is a VS Code extension for prettier: https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode. Feel free to add that to the devcontainer in a separate PR if you want.

[keves1]

How hard would it be to do late fusion, so pass each image through the encoder separately, then concatenate them, then pass them through the decoder?

@adamjstewart I think this is a remaining question to resolve on this PR, and I realized that this is already what torchgeo.models.FCSiamConc is doing, right? And FCSiamConc is one of the model options for this trainer. So maybe it makes sense to also have an option that concatenates the images before passing through the model (keeping the Unet how it is).

[adamjstewart]

Worth considering using https://github.com/Z-Zheng/pytorch-change-models

[isaaccorley]

@keves1 Could I jump in and help get this over the finish line?

[keves1]

@isaaccorley some help would be great, though I also want to do what I can. I see you made some changes to transforms, so that took care of the issue with _RandomNCrop? Is all that is left resolving the merge conflicts or do you see other changes needed too?

[isaaccorley]

@isaaccorley some help would be great, though I also want to do what I can. I see you made some changes to transforms, so that took care of the issue with _RandomNCrop? Is all that is left resolving the merge conflicts or do you see other changes needed too?

Left some review comments but lgtm once those are changed.

[keves1]

@isaaccorley I rebased to bring in your augmentation changes. It looks like _ExtractPatches doesn't work with the added temporal dimension (the images have shape BTCHW) - you can see the issue by running the tests in tests/trainers/test_change.py. Do you know how to fix that?

I'll work on your review comments.

[isaaccorley]

I'll take a look and se what can be done about running with the temporal dim.

[adamjstewart]

We need to upstream _RandomNCrop and _ExtractPatches to Kornia so we don't have to maintain them anymore. For now, if RandomCrop and CenterCrop work, I would be fine with using them until Kornia has a better option.

[keves1]

@isaaccorley I'm trying to get test coverage for predict_step in ChangeDetectionTask, but it looks like this is not being tested because there isn't a predict dataset defined in the OSCD datamodule. Is it valid to just define self.dataset = OSCD(split='test') in setup of the OSCD datamodule?

[isaaccorley]

@isaaccorley I'm trying to get test coverage for predict_step in ChangeDetectionTask, but it looks like this is not being tested because there isn't a predict dataset defined in the OSCD datamodule. Is it valid to just define self.dataset = OSCD(split='test') in setup of the OSCD datamodule?

Yes I think that should be sufficient.

[keves1]

@isaaccorley and @adamjstewart I think this is ready. Let me know what I should put in ChangeDetectionTask for versionadded.

[keves1]

SemanticSegmentationTask, on which this trainer is based, changed a lot in #2560, #2219, and #2690. We should update this PR with those same changes.

I could add the denormalization for plotting, but as far as adding support for multiclass and multilabel, we specifically decided that this PR would just support binary change detection, so I'd like to keep it like that and someone else can extend it to those cases later.

[hkristen]

SemanticSegmentationTask, on which this trainer is based, changed a lot in #2560, #2219, and #2690. We should update this PR with those same changes.

I could add the denormalization for plotting, but as far as adding support for multiclass and multilabel, we specifically decided that this PR would just support binary change detection, so I'd like to keep it like that and someone else can extend it to those cases later.

I could have a look at adding multiclass & multilabel support once this PR is merged. As I have done something similar already for my research. If you @adamjstewart would guide me a bit for my first PR to torchgeo on this one?

[adamjstewart]

@keves1 yes, let's add denormalizing and switch from if-statements to match-statements.

I'll let @hkristen take a stab at non-binary change detection in a follow-up PR after this is merged. I know @hfangcat is also interested in change detection for 3+ images, I'm not sure if that would be a feature to add to ChangeDetectionTask or SemanticSegmentationTask. I would like to merge this PR soon as I think it's almost ready, we can iterate on additional features later.

[keves1]

@adamjstewart I added denormalizing and switched to match statements, as well as made the other changes you suggested. I think it's ready now.

[keves1]

Looks like the way I did the monkeypatch to test predict_step when there isn't a predict dataset didn't work (it didn't run predict_step). What would be the right way to do this? I tried setting predict_dataloader and predict_dataset.

[adamjstewart]

Very close to being ready to merge, excited to finally get this in!!!