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

FCCD Models refactor #345

Merged
merged 13 commits into from
Feb 15, 2022
Merged

Conversation

isaaccorley
Copy link
Collaborator

This PR;

  • removes FCEF model since this is basically a U-Net with the 2 change images stacked together so the input channels are 2*num_channels.
  • Refactors FCSiamConc and FCSiamDiff to inherit from segmentation_models_pytorch.Unet. These models are basically U-Nets but feed both images through the U-Net encoder and then either take the difference (FCSiamDiff) or concatenate (FCSiamConc).

Notes:

  • By inheriting from smp this allows us to easily load different encoder backbone weights (I'll add a follow up PR to add this later). Previously the backbone was fixed.
  • For the models I'm reusing the smp.Unet constructor and it's args. For FCSiamDiff I made a dummy constructor so that I had a place to put the docstring. Not sure if this is the best way.

@isaaccorley isaaccorley added the models Models and pretrained weights label Dec 31, 2021
@isaaccorley isaaccorley self-assigned this Dec 31, 2021
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Dec 31, 2021
@github-actions github-actions bot added the testing Continuous integration testing label Jan 1, 2022
@isaaccorley
Copy link
Collaborator Author

@adamjstewart I'm getting the below errors in the docs. Do you know if there's a quick fix for these? I'm basically inheriting from some segmentation_models_pytorch classes and I think it's unable to find information from the parent classes.

generating indices... /home/docs/checkouts/readthedocs.org/user_builds/torchgeo/checkouts/345/torchgeo/models/__init__.py:docstring of torchgeo.models.FCSiamConc:1: WARNING: py:class reference target not found: segmentation_models_pytorch.base.model.SegmentationModel
/home/docs/checkouts/readthedocs.org/user_builds/torchgeo/checkouts/345/torchgeo/models/__init__.py:docstring of torchgeo.models.FCSiamDiff:1: WARNING: py:class reference target not found: segmentation_models_pytorch.unet.model.Unet

@adamjstewart
Copy link
Collaborator

@isaaccorley you need to add smp to the intersphinx_mapping in docs/conf.py.

@adamjstewart adamjstewart added this to the 0.3.0 milestone Jan 1, 2022
@isaaccorley
Copy link
Collaborator Author

Still can't seem to get this one to work. Wonder if it's similar to your issue with the PyTorch modules.

calebrob6
calebrob6 previously approved these changes Feb 15, 2022
docs/conf.py Outdated Show resolved Hide resolved
@calebrob6
Copy link
Member

Cool should be good now

@calebrob6 calebrob6 enabled auto-merge (squash) February 15, 2022 21:58
@calebrob6 calebrob6 merged commit 2c6e7ee into microsoft:main Feb 15, 2022
@adamjstewart
Copy link
Collaborator

When I make the 0.3.0 release notes, remind me to add this FCEF removal as a backward incompatible change in case anyone was relying on it.

Also, who was it who added these models? Should we ping them to review? (guess we should've done that before merging but it's not too late to undo anything if it's problematic)

@isaaccorley isaaccorley deleted the models/fccd-smp-refactor branch February 15, 2022 23:42
@adamjstewart adamjstewart added the backwards-incompatible Changes that are not backwards compatible label Jul 10, 2022
@adamjstewart adamjstewart mentioned this pull request Jul 11, 2022
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* remove FCEF model, refactor FCSiamConc and FCSiamDiff to inherit from smp.Unet

* style fixes update tests

* mypy and docstring fixes

* more mypy fixes and add tests

* remove test args

* fix tests

* add smp to intersphinx mapping

* update model DOI

* add the right docs this time

* Removing type ignores

* Testing sphinx fix

* Added parts of SMP to nitpicky ignore

* Fixing docs

Co-authored-by: Caleb Robinson <calebrob6@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-incompatible Changes that are not backwards compatible 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.

None yet

3 participants