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

Datamodule cleanup #657

Merged
merged 7 commits into from
Jul 9, 2022
Merged

Datamodule cleanup #657

merged 7 commits into from
Jul 9, 2022

Conversation

calebrob6
Copy link
Member

Cleaning up how preprocessing in the datamodules works in preparation for writing a prediction utility.

Now, each DataModule has a preprocess(self, sample) method that performs the image preprocessing and doesn't break if there isn't a "mask" or "label" key in the sample. This is important as predict.py needs to be able to apply the same image pre-processing that a trained model checkpoint used while training, however the new data that the model is being run over will definitely not have any "mask" or "label" parts.

I also took this opportunity to clean up a few things throughout the DataModules -- I'll go through and explain these changes inline with comments.

@github-actions github-actions bot added datamodules PyTorch Lightning datamodules testing Continuous integration testing labels Jul 7, 2022
conf/etci2021.yaml Show resolved Hide resolved
torchgeo/datamodules/chesapeake.py Show resolved Hide resolved
torchgeo/datamodules/chesapeake.py Show resolved Hide resolved

sample["image"] = sample["image"].float()
def remove_bbox(self, sample: Dict[str, Any]) -> Dict[str, Any]:
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing the "bbox" key is required for using the samples in the lightning trainer, however we need the "bbox" for predictions so I decoupled this logic from preprocess.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you remember why we had to remove bbox again? I feel like there was an option to keep it if we did something else differently...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm suspicious that it has something to do with collation, but the trainers/test definitely break if you don't remove it.

torchgeo/datamodules/cowc.py Show resolved Hide resolved
@@ -9,7 +9,7 @@
import pytorch_lightning as pl
import torch
from torch.utils.data import DataLoader
from torchvision.transforms import Compose, Normalize
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed Normalize as it is more explicit to subtract band means then divide by band stdevs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if I agree with this. I think the Normalize transform is more explicit since it tells you why you're doing it, not just what you're doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, I was thinking of it as, "there are many ways to 'normalize' something where Normalize happens to apply channel wise standardization". I don't have a strong opinion here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have a strong preference here either. I tend to use builtin library functions where possible since I trust them more than I trust my own coding, but this one is obviously very simple. I think the biggest advantage of using Normalize is that it does the .reshape(-1, 1, 1) here for you, so that's one less thing to worry about. It also uses .view(-1, 1, 1) which I think is more efficient. It also accepts a list, so you don't need to convert it to a tensor yourself. Personally I'm gonna vote for using Normalize to keep the code as simple as possible.

torchgeo/datamodules/so2sat.py Show resolved Hide resolved
torchgeo/datamodules/ucmerced.py Show resolved Hide resolved
@@ -24,7 +23,6 @@ def __init__(
self,
root_dir: str,
labels: Sequence[str] = USAVars.ALL_LABELS,
transforms: Optional[Callable[[Dict[str, Tensor]], Dict[str, Tensor]]] = None,
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't allow transforms to be passed in to any other DataModules

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is what we call a slippery slope: #498 (comment)

Also see #596

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha yeah I know; I can't make up my mind here. If we have this Optional[Callable] as an argument, then we can't (easily) initialize a class instance from a configuration file. This will make general-purpose scripts like predict.py more difficult to write. On the other hand, our datamodules aren't very useful for experimentation as-is, because good train-time augmentations are a very important.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we have a set of default transforms associated with each datamodule? Then users can override them in Python if they need to via this optional callable (experimentation) but we'll still have config files without customization that default to the optimal known transforms (reproduction).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think that's the best of both worlds. I don't think an "optimal" known transform exists across these datasets -- but a sane default implemented consistently would be a good step. Further, as long as these are optional train transforms, instantiating the DataModule without them shouldn't have any effect on using the DataModule for pre-processing purposes.

@adamjstewart adamjstewart modified the milestone: 0.3.0 Jul 7, 2022
preprocessed sample
"""
sample["image"] = sample["image"].float()
sample["image"] /= 255.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like we're somewhat arbitrarily deciding whether to normalize to [0, 1] or [-1, 1]. Not sure if there is any advantage to one or the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep!

Copy link
Member Author

@calebrob6 calebrob6 Jul 9, 2022

Choose a reason for hiding this comment

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

I wouldn't mind arbitrarily being consistent and doing [0,1] normalization for everything.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if this needs to be solved in this PR, just something to think about. We should probably try both and see if it makes any difference. It may be different on a case-by-case basis.

@calebrob6
Copy link
Member Author

Okay, so I'm going to switch to using Normalize consistently then leave further changes for another PR

adamjstewart
adamjstewart previously approved these changes Jul 9, 2022
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.

Technically, a lot of these changes (transforms -> preprocess) are API changes that should be documented with .. versionchanged:: 0.3 but I doubt too many people are calling these directly or overriding them so I won't ask for documentation of these changes.

Inria adds an augmentations parameter. Do we want to remove this now for consistency or keep it and think about how to handle this later? Inria was added in this release so its API hasn't been public so we can remove it without documenting the change if we do it now.

@calebrob6
Copy link
Member Author

Inria adds an augmentations parameter. Do we want to remove this now for consistency or keep it and think about how to handle this later? Inria was added in this release so its API hasn't been public so we can remove it without documenting the change if we do it now.

I think remove it

@adamjstewart adamjstewart merged commit db3f183 into main Jul 9, 2022
@adamjstewart adamjstewart deleted the datamodule_cleanup branch July 9, 2022 20:59
@adamjstewart adamjstewart mentioned this pull request Jul 11, 2022
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* Cleaning up preprocessing methods across DataModules

* Decoupled deleting the bbox with the other transforms in the GeoDataset DataModules

* Cleaning up how channel standardization is done

* Changing default conf for ETCI2021 and fixing So2Sat

* Forgot to update the indices.

* Change to use Normalize

* Remove default augs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datamodules PyTorch Lightning datamodules testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants