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

Move DataModules to torchgeo.datamodules #321

Merged
merged 2 commits into from
Dec 24, 2021
Merged

Conversation

adamjstewart
Copy link
Collaborator

This is a pretty disruptive change to our API, and a lot of work to move everything, so I'll do my best to justify this. I'm hoping that this is the last time we have to move these.

Originally, all of our DataModules were defined in torchgeo.trainers. However, we decided to move them to torchgeo.datasets in #220 since the filenames had so much overlap with the dataset filenames (ideally there should be a one-to-one correspondence between Datasets and DataModules). This introduced a circular import issue (#276) because our samplers import GeoDataset/BoundingBox and our DataModules import our samplers. Although there are workarounds for this, circular import issues are generally a sign of a poor design. In this case, the torchgeo.* components were not standalone and distinct enough.

This PR solves the issue by moving all DataModules to their own torchgeo.datamodules namespace. Now our components have the following import dependence:

As you can see, no more circular import dependencies. Aside from avoiding circular imports, the other motivation of this PR is that it allows us to remove all imports in torchgeo/__init__.py. Some of those imports were hacks to avoid circular import issues. I'll remove these imports in a separate PR to keep this one focused.

@adamjstewart adamjstewart added the datasets Geospatial or benchmark datasets label Dec 22, 2021
@adamjstewart adamjstewart added this to the 0.2.0 milestone Dec 22, 2021
@adamjstewart adamjstewart requested review from calebrob6 and isaaccorley and removed request for calebrob6 December 23, 2021 18:31
"RESISC45DataModule",
"SEN12MSDataModule",
"So2SatDataModule",
"CycloneDataModule",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be moved above to the C section?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, it's sorted under "Tropical Cyclone". I think we may want to consider renaming the DataModule to be more consistent with the Dataset. But let's do that another day.

@adamjstewart adamjstewart merged commit cbebc1e into main Dec 24, 2021
@adamjstewart adamjstewart deleted the torchgeo/datamodules branch December 24, 2021 02:10
@adamjstewart adamjstewart added utilities Utilities for working with geospatial data and removed utilities Utilities for working with geospatial data labels Jan 2, 2022
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* Move DataModules to torchgeo.datamodules

* Clean up local imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datasets Geospatial or benchmark datasets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants