torchgeo icon indicating copy to clipboard operation
torchgeo copied to clipboard

fix SeCo transforms

Open isaaccorley opened this issue 2 years ago • 11 comments

The current SeCo transforms do the following:

  • resize
  • center crop
  • normalize using the quantiles
  • multiply by 255
  • normalize using imagenet stats

However in the script they actually do the following:

  • resize
  • center crop
  • normalize using the quantiles
  • multiply by 255
  • clip to [0. 255]
  • convert to uint8
  • divide by 255
  • normalize using imagenet stats

isaaccorley avatar May 11 '23 21:05 isaaccorley

Whether you agree or not with the changes, our current seco transforms are incorrect. I'm just taking what they have from their script.

isaaccorley avatar May 11 '23 23:05 isaaccorley

I don't actually know how to fix the mypy errors other than ignoring them.

  • I can't type hint a lambda but kornia doesn't have a clip augmentation.
  • K.AugmentationSequential complains about the K.contrib.Lambda input arg

isaaccorley avatar May 11 '23 23:05 isaaccorley

Whether you agree or not with the changes, our current seco transforms are incorrect. I'm just taking what they have from their script.

In my mind, the transforms supplied with the weights aren't designed to reproduce SeCo pre-training, they're designed to use the SeCo weights for downstream tasks. If these changes don't improve performance on downstream tasks and only make transforms slower, or even harm performance on downstream tasks, then we shouldn't add them. That's why an ablation study is needed to ensure that these changes are beneficial.

adamjstewart avatar May 11 '23 23:05 adamjstewart

In that case then the transforms are completely wrong, because SeCo doesn't use these for downstream tasks at all. They only divide eurosat by 255, they use different BigEarthNet mean and std. They also don't resize to 224x224. I would argue that you would need to show an ablation study of why you shouldn't use the same transforms as those used during training since that's less intuitive. It's trained to take 8-bit inputs so I would expect performance would degrade if you feed it 12-bit inputs.

isaaccorley avatar May 11 '23 23:05 isaaccorley

Let's hold off on this PR until after the deadline so we have time to do a proper ablation study on no transforms, current transforms, and proposed transforms.

adamjstewart avatar May 11 '23 23:05 adamjstewart

I'm good with this just wanted to use it for comparisons to SeCo

isaaccorley avatar May 11 '23 23:05 isaaccorley

Added this to the 0.4.2 milestone so we don't forget about it. At the bare minimum, we should remove the multiply by 255 stuff. The rest is more controversial and will need ablation studies that we don't have time to work on until after the deadline.

adamjstewart avatar May 25 '23 03:05 adamjstewart

Is this still a work in progress?

adamjstewart avatar Feb 29 '24 13:02 adamjstewart

@adamjstewart I can finish this today. It fell off my radar.

isaaccorley avatar Feb 29 '24 13:02 isaaccorley

No rush. If we can squeeze it into 0.5.2, great. If not, we can continue to punt it. Just trying to finish up old PRs.

adamjstewart avatar Feb 29 '24 15:02 adamjstewart