functionality for learning on the prior with QR loss and ChesapeakeCVPR data

[estherrolf]

No description provided.

[ghost]

All CLA requirements met.

[calebrob6]

Context: PR to add a trainer, new loss function, and an update to the ChesapeakeCVPR dataset that includes prior estimates of land cover derived from weak supervision. This is for reproducibility of a paper under review.

@estherrolf, looks great, thank you so much! I can take it from here more/less, will check with you offline for major changes and to assert that the results look similar.

Things on my radar for any other reviewers that want to jump in:

• Merge the ChesapeakeCVPR datasets
• Remove FCN_modified (and use FCN in the trainer)
• Satisfy mypy ridiculousness
• Add to docs where appropriate
• Copy the testing template from the other PR

[isaaccorley]

I started reviewing but seems like you're going to take over @calebrob6

[adamjstewart]

@calebrob6 converted to a draft, just mark as "Ready for review" once this is ready

[calebrob6]

@adamjstewart @isaaccorley this is ready for a review pass if you have the time (ignoring the code in the trainer file for now).

Summary:

• Adds an extension to the Chesapeake CVPR dataset with per pixel land cover priors from https://zenodo.org/record/5652512
• Adds the QR and RQ losses described in https://openreview.net/forum?id=AEa_UepnMDX

@estherrolf, questions:

• can we move the trainer to the independent code repo? (we're trying to minimize the amount of custom trainer code that lives directly in torchgeo)
• can we talk through the args in ChesapeakeCVPRPriorDataModule? (I want to make sure I understand the purpose of everything before I change more)

[estherrolf]

@calebrob6 re: trainers yes, I've moved the chesapeake_train_on_prior.py as well as all the envioratlas trainers to my experimentation/paper repo so feel free to remove or edit those, whichever makes sense. I also will instantiate the "modified" fcns in my experimentation repo so don't feel any pressure to integrate those unless you want to for some other reason.

re: ChesapeakeCVPRPriorDataModule i think we sorted that out in a call just now, but lmk if questions arise!

[calebrob6]

@estherrolf this one is finished (after code review)! I ended up merging the ChesapeakeCVPRPriorDataModule with the existing ChesapeakeCVPRDataModule. If you pass use_prior_labels, then it will return batches where the "mask" is the prior labels, else it will return batches where the "mask" is the high-res labels as usual (also carried over the spatial smoothing parameter).

Before removing the old DataModule, I asserted that this new merged one gives the same outputs, e.g. both return this image as the "tree" prior for the same bounding box in de-train (the blank area is because of padding):

Of note, in the old ChesapeakeCVPRPriorDataModule you returned the prior as batch["mask"] and the high-res labels as batch["high_res_labels"], however I dropped the "high_res_labels" for consistency with other DataModules (also training should go faster if you don't load and extra layer with each patch). If you really want this for visualization during training I can show you how to make a new DataModule that extends this class and does that (it would only be a few lines).

[adamjstewart]

I'm hesitant to add these loss functions to TorchGeo. From what I can tell from reading the paper, these loss functions are not specific to geospatial data. I want to keep TorchGeo as focused on geospatial data as possible for two reasons:

1. Maintainer burden: we don't want to maintain things that we don't need to
2. Sharing: we want as many people to benefit from these functions as possible, not just geoscientists

Can these functions instead be contributed to PyTorch/torchvision/Kornia?

[calebrob6]

these loss functions are not specific to geospatial data

Do you know of any loss functions that are specific to geospatial data that are candidates here? Off the top of my head the geospatial ML specific papers we've talked about would not fit:

• Tile2Vec is a standard triplet loss with custom sampling strategy.
• Geography Aware SSL uses InfoNCE but uses time to select samples. They also have predicting lat, lon as a pretext task, however they bin the location and use normal classification class.
• SeCo also uses InfoNCE but uses time to select samples.

Maintainer burden: we don't want to maintain things that we don't need to

Can you explain this further?

Sharing: we want as many people to benefit from these functions as possible, not just geoscientists

Implementing QR / RQ loss here doesn't preclude anyone else from implementing it (and I'd argue gives it a higher likelihood of being noticed by someone that might want to use it). Maybe I don't understand this point either?

Can these functions instead be contributed to PyTorch/torchvision/Kornia?

Maybe, but I definitely don't have the bandwidth to learn how to contribute to pytorch. E.g. I tried to fix a basic typing issue with F.normalize (that affects this PR) the other day and a large number of tests failed cryptically pytorch/pytorch#70149 😄. Something with JIT.

[isaaccorley]

Should we consider making separate datasets and datamodules for the original dataset and the one with the priors? If I'm a user and I just want the vanilla ChesapeakeCVPR dataset, I'd imagine this would be harder to parse what's going on with all the additions in this PR.

Edit: actually since this just adds another layer to the dataset maybe it's fine as is.

[adamjstewart]

Do you know of any loss functions that are specific to geospatial data that are candidates here?

Nope, that's why we've never proposed a torchgeo.losses module before.

Maintainer burden: we don't want to maintain things that we don't need to

Can you explain this further?

Maintainer burden is the idea that the more code you add to a project, the more you have to maintain. When you add a feature to a library like TorchGeo, you're making a promise to the user that this feature works. If a user discovers a bug in that feature, the burden is on the maintainer to fix it. In the future, once TorchGeo becomes more stable, it's also a promise that this feature will exist and won't disappear all of a sudden. If we need to refactor (for example, function -> nn.Module) at a later date, this becomes more complicated the more features we have to refactor.

We're starting to see this with our datasets. We have a ton of datasets now, which is awesome, but we also decided after the fact that all datasets should have a plot function and should allow already downloaded tarballs. This is quite a bit of work to do for all datasets, and would be much worse if we also decided to include MNIST/CIFAR/ImageNet/etc.

If you recall the torchvision PRs I did a while back, most of them were rejected not because they weren't useful, but because they increased maintainer burden on the torchvision devs for something that wasn't directly useful for their project.

Sharing: we want as many people to benefit from these functions as possible, not just geoscientists

Implementing QR / RQ loss here doesn't preclude anyone else from implementing it (and I'd argue gives it a higher likelihood of being noticed by someone that might want to use it). Maybe I don't understand this point either?

For our paper, one of our reviewers pointed out that many of the problems associated with remote sensing imagery are not unique to remote sensing, they are also found in the biomedical domain. Our response to that question was basically what I'm suggesting here, that transforms specific to geospatial data belong in TorchGeo, while transforms that are useful for any kind of multispectral image data belong in libraries like Kornia/torchvision. There are far more people who use Kornia/torchvision than there are TorchGeo. If every library is forced to reimplement this same feature, you end up with a lot of code duplication, which is bad.

I'm thinking about these loss functions the same way I would if someone wanted to contribute a new dataset or model to TorchGeo that had absolutely nothing to do with geospatial data. Another example would be if you wanted a metric that wasn't already in torchmetrics. I wouldn't want to accept that into TorchGeo because it really belongs in torchmetrics. This is in line with the Unix philosophy of doing one thing well.

Basically, I'm not 100% opposed to adding non-geospatial-specific datasets/models/losses/transforms to TorchGeo, but I'm going to need quite a bit of convincing as to why they have to be in TorchGeo before I commit to maintaining them. Also, as soon as we accept one non-geospatial-specific feature, that sets a precedent for adding more in the future.

[adamjstewart]

Can these functions instead be contributed to PyTorch/torchvision/Kornia?

Maybe, but I definitely don't have the bandwidth to learn how to contribute to pytorch.

Another option is to move these loss functions to your independent code repo until someone has the bandwidth to contribute them to one of these libraries.

[adamjstewart]

Before I forget, we need to add losses to pyproject.toml under [tool.pydocstyle].

[calebrob6]

For clarity to anyone reading this, we discussed this offline and decided to include the losses. There are several losses that, while not created exclusively for geospatial data, are particularly useful with geospatial data and that aren't implemented elsewhere. We feel that these are in scope for torchgeo until/unless they are picked up by a general-purpose library (e.g. a torchmetrics for loss functions would be an awesome place for these 😄).

[calebrob6]

@adamjstewart, rebased to accommodate the new datamodule organization, fixed previous changes

[adamjstewart]

I think this is pretty close, just had some comments on tests and naming