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

Add RESISC45 Trainer #179

Merged
merged 9 commits into from
Oct 11, 2021
Merged

Conversation

isaaccorley
Copy link
Collaborator

@isaaccorley isaaccorley commented Sep 30, 2021

  • Adds RESISC45DataModule, RESISC45ClassificationTask
  • Adds torchgeo.datasets.utils.dataset_split for creating train/val or train/val/test subsets of a dataset with no split provided beforehand.
  • Adds unit tests for torchgeo.datasets.utils.dataset_split

Notes:

  • Will add unit tests for trainer in separate PR

@isaaccorley isaaccorley marked this pull request as draft September 30, 2021 21:03
calebrob6
calebrob6 previously approved these changes Oct 8, 2021
@isaaccorley isaaccorley marked this pull request as ready for review October 9, 2021 00:45
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.

This is the last trainer I'm accepting without unit tests :)

torchgeo/datasets/utils.py Show resolved Hide resolved
@adamjstewart adamjstewart added the trainers PyTorch Lightning trainers label Oct 9, 2021
@isaaccorley
Copy link
Collaborator Author

isaaccorley commented Oct 10, 2021

This is the last trainer I'm accepting without unit tests :)

I agree. I think we should merge this after I add unit tests for dataset_split because we used this for experiments in the paper. I'll figure out the unit tests in a separate PR and then it will be mostly copy/paste unit tests for the other trainers.

At some point we should refactor the trainers to inherit from base lightning module classes as well for classification and segmentation tasks since most of them are copy/paste so if you make a small change to one classification trainer task you generally have to do this to all other classification trainers.

@adamjstewart
Copy link
Collaborator

Yeah, the trainers and experiment scripts are all almost identical. I thought the whole point of the trainers was to reduce code duplication lol.

@isaaccorley
Copy link
Collaborator Author

I did something similar in torchrs by making a base class for lightning modules and datamodules and then just inheriting. It made adding new trainers straightforward which I think should be the goal.

@calebrob6
Copy link
Member

calebrob6 commented Oct 10, 2021

I agree that we should go back and write unit tests for the existing trainers -- I'll open a PR with unit tests for one of them (unless Isaac beats me to it) and we can work from there

@adamjstewart
Copy link
Collaborator

See #109 as a good starting point. I got most of the way there.

@calebrob6 calebrob6 merged commit 142835c into microsoft:main Oct 11, 2021
@adamjstewart adamjstewart added this to the 0.1.0 milestone Nov 20, 2021
@isaaccorley isaaccorley deleted the trainers/resisc45 branch January 7, 2022 23:54
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* add RESISC45 trainer

* update working locally

* Adding ability to choose the random split sizes via config

* If you don't have a val or test split, then return the train split so the Trainer doesn't break by default. If you actually want to train without val/test though, then you should set the appropriate Trainer args.

* RESISC experiments

* Reverting accidental changes

* mypy fix

* add dataset_split unit tests

* Document dataset_split

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
trainers PyTorch Lightning trainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants