pytorch · NicolasHug · Dec 6, 2021 · Dec 2, 2021 · Dec 2, 2021 · Dec 2, 2021
diff --git a/torchvision/models/__init__.py b/torchvision/models/__init__.py
@@ -12,6 +12,7 @@
 from .regnet import *
 from . import detection
 from . import feature_extraction
+from . import optical_flow
 from . import quantization
 from . import segmentation
 from . import video
diff --git a/torchvision/models/optical_flow/__init__.py b/torchvision/models/optical_flow/__init__.py
@@ -0,0 +1 @@
+from ._raft.raft import RAFT, raft, raft_small
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		from ._raft.raft import RAFT, raft, raft_small
Copy link Member Author NicolasHug Dec 2, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. We should expose the building blocks as well, stuff like `ResidualBlock`, `FeatureEncoder`, etc. Should we expose these in a `models.optical_flow.raft` namespace? I remember @datumbox mentioning a few issues when we have both a module and a function with the same name. Ideally, I'd like to keep the `raft()` name for the function builder, but I'm happy to get your thoughts on this Copy link Contributor datumbox Dec 2, 2021 • edited Loading Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Blocking: See @pmeier's #4867 (comment) around this. We might need to rename this to `raft_large` similar to `mobilenet_v3_large` and `mobilenet_v3_small`. Question: Can you talk about the choice to create a second private submodule `_raft`? Why is that? FYI: We should expose the building blocks as well, stuff like ResidualBlock, FeatureEncoder, etc. If you check existing models, such as resnet and faster_rcnn you will see we don't expose these on the `__all__` or anywhere else. We had discussions about what this means but we didn't have a consensus (public API vs developer API discussion). I would be in favour of not exposing these publicly to be consistent with everywhere else. We should discuss and resolve this once we have time in the roadmap. Copy link Member Author NicolasHug Dec 2, 2021 • edited Loading Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Thanks for the feedback Can you talk about the choice to create a second private submodule _raft? Why is that? I wrote it like this out of habit but I had no intention of keeping it this way Instead of renaming `raft()` to `raft_large()`, would it be OK instead to rename the `.py` file to something like `raft_core.py` ? Or `raft_implem.py`? We would have `raft()`, `raft_small()` and `RAFT` available from `torchvision.models.optical_flow` and then we would be able to access the other building blocks like `ResidualBlock` in `torchvision.models.optical_flow.raft_core` -- I would make sure to exclude them from `__all__`. Would that be OK? Copy link Contributor datumbox Dec 2, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Though calling it `raft_large` would make it consistent with other small/large models, I can see that on the paper they don't actually call it large. They do use `Raft-s` or "small" for the small version, so that's canonical at least. So I understand why you want to avoid naming it like that. Renaming the file should be OK but @pmeier should confirm. Though I'm not sure what the actual name should be. We typically name the files after their algorithm and we let the model builder have extra info on the variant. @fmassa thoughts on this? Copy link Member Author NicolasHug Dec 2, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. OK, I'll use `raft.py` and `raft_large()` for now to move forward. We can revisit later if needed, thanks for the input Copy link Collaborator pmeier Dec 2, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Not commenting on the name, because I don't have an opinion on that. We just need to make sure that we don't have an attribute with the same name as a module in the same namespace. Otherwise we can no longer access the module. Thus having `raft.py` and `def raft()` is problematic, but `raft.py` and `raft_large()` is fine.