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

Models: fix preprocessing transforms #1166

Merged
merged 5 commits into from
Mar 23, 2023
Merged

Models: fix preprocessing transforms #1166

merged 5 commits into from
Mar 23, 2023

Conversation

adamjstewart
Copy link
Collaborator

Our pre-trained models come with preprocessing transforms that should be used to normalize and reshape images used with them. However, these transforms don't actually run. This PR fixes them and adds tests to ensure that all transforms actually work.

@adamjstewart adamjstewart added this to the 0.4.1 milestone Mar 9, 2023
@github-actions github-actions bot added models Models and pretrained weights testing Continuous integration testing labels Mar 9, 2023
818.86747235,
]
),
mean=torch.tensor([590.23569706, 614.21682446, 429.9430203]),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We only have weights for RGB images

]
),
mean=torch.tensor([590.23569706, 614.21682446, 429.9430203]),
std=2 * torch.tensor([675.88746967, 582.87945694, 572.41639287]),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@nilsleh nilsleh Mar 16, 2023

Choose a reason for hiding this comment

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

https://github.com/ServiceNow/seasonal-contrast/blob/8285173ec205b64bc3e53b880344dd6c3f79fa7a/datasets/bigearthnet_dataset.py#L111 Is this also different from the Kornia Normalize method, as they use some min max operation and also clip to range [0, 255]? Or is this something like percentiles used to normalize? Never seen this tbh. I guess my question is does the difference matter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, this is actually quite different. They substract (mean - 2 * std), not mean, and they divide by (4 * std), not std. I have no idea why they scale by 255, seems odd unless you're trying to plot something. And we obviously aren't clamping (again, only useful for plotting).

EDIT: oh, is it because they need to load it as a PIL image? Maybe that removes the 255 when converting to a Tensor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure how they're even using PIL here since it only supports RGB, not MSI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I guess that's why SeCo only had RGB weights?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes, they divide by 10K * 255 for S1 but not for S2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The more I look at these, the less sure I am that they are correct. May want to reach out to the authors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Once we figure this out, we should also fix the SeCO datamodule I recently added. According to @wangyi111, it actually doesn't matter that much what the exact normalization is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opened an issue to get to the bottom of this. I actually think we're using the wrong normalization (this was only used for BigEarthNet, not for any other dataset). My guess is the SeCo normalization, but we'll see if we get a response. If I don't hear anything back, let's just assume SeCo normalization.

ServiceNow/seasonal-contrast#19

@adamjstewart adamjstewart requested a review from nilsleh March 9, 2023 19:26
isaaccorley
isaaccorley previously approved these changes Mar 10, 2023
@adamjstewart adamjstewart marked this pull request as draft March 18, 2023 16:49
@adamjstewart adamjstewart force-pushed the fixes/model-transforms branch from 858cc65 to 397265a Compare March 18, 2023 23:01
@github-actions github-actions bot added the datamodules PyTorch Lightning datamodules label Mar 18, 2023
@adamjstewart adamjstewart marked this pull request as ready for review March 18, 2023 23:02
nilsleh
nilsleh previously approved these changes Mar 19, 2023
calebrob6
calebrob6 previously approved these changes Mar 22, 2023
@adamjstewart adamjstewart dismissed stale reviews from calebrob6 and nilsleh via 5471224 March 22, 2023 16:59
@calebrob6 calebrob6 merged commit 88df515 into main Mar 23, 2023
@calebrob6 calebrob6 deleted the fixes/model-transforms branch March 23, 2023 04:59
calebrob6 pushed a commit that referenced this pull request Apr 10, 2023
* Models: fix preprocessing transforms

* Fix normalization of SeCo std dev

* black

* Fix SeCo transforms

* Add comment explaining source of transforms
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* Models: fix preprocessing transforms

* Fix normalization of SeCo std dev

* black

* Fix SeCo transforms

* Add comment explaining source of transforms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datamodules PyTorch Lightning datamodules models Models and pretrained weights testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants