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 LoveDADataModule to the trainer tests #966

Merged
merged 8 commits into from
Dec 30, 2022
Merged

Conversation

calebrob6
Copy link
Member

@adamjstewart the problem with LoveDA you likely ran into is that the test set doesn't have any masks, just imagery (which we accurately reflected when creating the test data)

@github-actions github-actions bot added scripts Training and evaluation scripts testing Continuous integration testing labels Dec 21, 2022
@adamjstewart
Copy link
Collaborator

Looks great! I'll try to figure out the failing tests, enjoy your vacation!

@calebrob6
Copy link
Member Author

Minimum tests have a pytorchlightning warning that gets treated as an error, I tried to ignore it but messed something up

@adamjstewart
Copy link
Collaborator

Just noticed that our data loader made their news: https://github.com/Junjue-Wang/LoveDA#news

@adamjstewart
Copy link
Collaborator

Okay, so looking into this more, I think it's strange to have a datamodule where test doesn't report an accuracy. I think we should instead split val (dataset) into val/test (datamodule) and move test (dataset) to predict (datamodule). We also use test during predict in the Inria datamodule, so there is precedence for this.

In terms of split sizes, we have:

  • train: 2522
  • val: 1669
  • test: 1796

I think it makes the most sense to split val in half so that we have enough samples to train on and can still evaluate val/test accuracy.

@nilsleh @Junjue-Wang do you have any opinions on this?

@Junjue-Wang
Copy link

Okay, so looking into this more, I think it's strange to have a datamodule where test doesn't report an accuracy. I think we should instead split val (dataset) into val/test (datamodule) and move test (dataset) to predict (datamodule). We also use test during predict in the Inria datamodule, so there is precedence for this.

In terms of split sizes, we have:

  • train: 2522
  • val: 1669
  • test: 1796

I think it makes the most sense to split val in half so that we have enough samples to train on and can still evaluate val/test accuracy.

@nilsleh @Junjue-Wang do you have any opinions on this?

Accurately, The test scores can be evaluated on the Codalab: https://codalab.lisn.upsaclay.fr/competitions/421#results
I don't think rearranging the data splits is a good solution.
We should align with the public benchmark.

@adamjstewart
Copy link
Collaborator

Will the test set masks ever be made public? If we can only make predictions on test and can't evaluate them locally then the datamodule becomes pretty limited.

@calebrob6
Copy link
Member Author

I don't think so, even without test set masks you need the data module for training with torchgeo which is a massive plus. it will also make it easy to generate test predictions.

@adamjstewart
Copy link
Collaborator

adamjstewart commented Dec 24, 2022

Another option would be the following mapping:

Dataset DataModule
train train
val val
val test
test predict

This feels a bit redundant, although it's what inria does when test_split_pct == 0.

@adamjstewart
Copy link
Collaborator

Another option would be:

Dataset DataModule
train train
val val
test predict

The datamodule wouldn't have a test_dataloader since there's no way to evaluate it anyway.

@github-actions github-actions bot added the datamodules PyTorch Lightning datamodules label Dec 25, 2022
@adamjstewart
Copy link
Collaborator

Depends on #975 to get the tests to pass.

@adamjstewart adamjstewart mentioned this pull request Dec 26, 2022
12 tasks
@adamjstewart adamjstewart merged commit 867a3bc into main Dec 30, 2022
@adamjstewart adamjstewart deleted the datamodule-loveda branch December 30, 2022 03:46
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* Add loveda to trainer tests

* Delete direct loveda datamodule test

* Ignoring deprecation warning for lightning

* Remove ignore

* test -> predict

* Fix typo

* Add comment explaining mismatch

* More coverage

Co-authored-by: Adam J. Stewart <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datamodules PyTorch Lightning datamodules scripts Training and evaluation scripts testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants