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

Panopticon Model #2692

Merged
merged 28 commits into from
Apr 5, 2025
Merged

Panopticon Model #2692

merged 28 commits into from
Apr 5, 2025

Conversation

LeWaldm
Copy link
Contributor

@LeWaldm LeWaldm commented Mar 31, 2025

Hi all,

This PR adds the Panopticon model to torchgeo.

Since Panopticon already has a working hubconf, we do not copy the code to torchgeo but just use the torch.hub.load to initialise the model & load the weights. 3 thoughts:

  1. We don’t really need the Panopticon_Weights class that the other models have. I still added it for completeness. Maybe we can drop it in the case of Panopticon?
  2. There is not a lot to test, since essentially no code is added to torchgeo. I still copied over a basic forward pass test from our repo.
  3. We still need to fill in 2 [TBD] in torchgeo/models/panopticon.py & add the panopticon weights to the torchgeo hugging face.

Happy about any feedback & how to proceed!

@github-actions github-actions bot added documentation Improvements or additions to documentation models Models and pretrained weights testing Continuous integration testing labels Mar 31, 2025
@LeWaldm
Copy link
Contributor Author

LeWaldm commented Mar 31, 2025

@microsoft-github-policy-service agree

I am not part of a company but this model was developed together with the other co-authors listed here https://arxiv.org/abs/2503.10845.

@isaaccorley
Copy link
Collaborator

Thanks for the PR @LeWaldm!

I haven't looked the underlying code yet, but curious if there is a way to modify the image size easily by interpolating the pos embed? Would be nice to be able to pass some extra args to the model to do so.

@adamjstewart adamjstewart added this to the 0.7.0 milestone Apr 1, 2025
@adamjstewart adamjstewart self-assigned this Apr 1, 2025
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.

An interesting idea that would allow us to add more models under non-MIT licenses!

However, we can't easily get test coverage without downloading something, which we don't want to do in our unit tests. It also doesn't seem possible to instantiate the model with random weights (for reproducibility or experimentation).

Let me know if you want help with the unit tests.

LeWaldm and others added 5 commits April 1, 2025 23:19
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
@LeWaldm
Copy link
Contributor Author

LeWaldm commented Apr 1, 2025

From ruff check I am still getting torchgeo/models/panopticon.py:1:1: D100 Missing docstring in public module but don't understand what that means.

Regarding unit tests:

  • what: I am not sure what to test. Torchgeo does not have a unified model API that we need to conform with. We ensure in our repo that the implementation of panopticon is correct. The only reasonable test I can think of is ensuring that the forward pass works and outputs correct shapes, as I already added.
  • how: I could add loading the model without weights via torchhub, however, again with download of the panopticon repo (a lot smaller than the ckpt though) in the background. The only quick way during testing would be to add all the source code into panopticon.py. Is that what we want compared to a single line of torch.hub.load(...)?

@LeWaldm
Copy link
Contributor Author

LeWaldm commented Apr 1, 2025

@isaaccorley the embeddings are automatically interpolated to the given image! I only wanted to highlight that inputting images of shape 224x224 probably gives best results since 224x224 was the shape during pre-training.

@adamjstewart
Copy link
Collaborator

From ruff check I am still getting torchgeo/models/panopticon.py:1:1: D100 Missing docstring in public module but don't understand what that means.

This means the file needs a docstring explaining what the file is. Something like:

"""Panopticon model."""

at the top of the file will fix this.

Torchgeo does not have a unified model API that we need to conform with.

TorchGeo follows the torchvision model API design. All of our existing models should be uniformly implemented. See DOFA or Copernicus-FM for examples.

The only quick way during testing would be to add all the source code into panopticon.py. Is that what we want compared to a single line of torch.hub.load(...)?

I would personally prefer adding the model code since it makes it easy to document, modify, and experiment with, but it's up to you. I can monkeypatch torch.hub.load to get fake coverage, but I would much rather test the model itself.

We ensure in our repo that the implementation of panopticon is correct.

On how many Python versions, OSes, and PyTorch versions? Does your repo have documentation of function and class parameters, static type hints, etc? Software is a lot more than just code. We frequently get bug reports for DOFA and other models that allow us to improve the models.

@LeWaldm
Copy link
Contributor Author

LeWaldm commented Apr 2, 2025

I created a file that contains all the code for panopticon (didn't push it yet, here to run panopticon. It has ~1000 lines and 23 functions / classes to potentially test.

I will not be able to test all of these. I might be able to write tests just for the new code from Panopticon (7 classes / functions). Would that make sense or do you need test for everything? How do we continue from here?

@adamjstewart
Copy link
Collaborator

We likely don't need that entire file. I'm guessing most of these classes can be imported from timm. We explicitly can't copy anything from DINOv2 since it's under a different license. So we would only include things unique to Panopticon and import the rest.

Don't worry about testing, I can handle that.

@LeWaldm
Copy link
Contributor Author

LeWaldm commented Apr 3, 2025

So dinov2 also has a torchhub. Would a potential middle ground be: load dinov2 via torchhub (also doesn't add any dinov2 code to torchgeo) and add all new panopticon stuff to torchgeo with tests?

@LeWaldm
Copy link
Contributor Author

LeWaldm commented Apr 3, 2025

Then, we could test the panopticon PE separately with quick tests and only have one slow test with a fwd pass of the whole model

@adamjstewart
Copy link
Collaborator

Timm also has DINOv2 ViTs: https://github.com/huggingface/pytorch-image-models/blob/v1.0.15/timm/models/vision_transformer.py#L3145

@LeWaldm
Copy link
Contributor Author

LeWaldm commented Apr 3, 2025

I will load dinov2 with timm and add the panopticon code. Will be done in next 2h.

@LeWaldm
Copy link
Contributor Author

LeWaldm commented Apr 3, 2025

I just pushed the code where basic functionality is working. However, there still is a lot of documentation to do. I will do that after dinner today.

I have not written any tests yet. If you can write some tests @adamjstewart , we might be able to make it until tomorrow. Else, we will need to wait for the next release.

@adamjstewart
Copy link
Collaborator

Yes, I can handle testing. I'm planning on writing release notes all day tomorrow, so if things still aren't finished when I'm done I'll jump in and finish them up. It already looks pretty close, I'm not worried about waiting a little for this PR since it will be a good way to advertise the paper.

@LeWaldm
Copy link
Contributor Author

LeWaldm commented Apr 3, 2025

I added documentation. The timm vision transformer is slightly different. Hence, the model is close but not exactly identical to our codebase. I think that is fine for now. Should we advertise that somewhere?

How do we continue with the tests? What should be tested?

@adamjstewart
Copy link
Collaborator

Is the model similar enough that the pretrained weights still load?

We need to test every single line of code to get 100% coverage. See the DOFA and Copernicus tests for examples.

@LeWaldm
Copy link
Contributor Author

LeWaldm commented Apr 4, 2025

I (with copilot) wrote some tests.

I can load the model weights, but the results are slightly different from our implementation. I investigate that later today. Apart from that, everything should be ready to go!

@github-actions github-actions bot added the dependencies Packaging and dependencies label Apr 4, 2025
@github-actions github-actions bot removed the dependencies Packaging and dependencies label Apr 4, 2025
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.

@LeWaldm can you do one last check and make sure I didn't break anything? Let me know if you have any questions about any of my changes.

@adamjstewart
Copy link
Collaborator

Merging due to release deadline, but let us know if there are any bugs that need to be fixed in the 0.7.1 release.

@adamjstewart adamjstewart merged commit fe1d75d into microsoft:main Apr 5, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation models Models and pretrained weights testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants