-
Notifications
You must be signed in to change notification settings - Fork 297
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
Adding ETCI2021 datamodule and trainer #234
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be added to docs
@@ -77,19 +77,28 @@ def __init__(self, **kwargs: Any) -> None: | |||
in_channels: Number of channels in input image | |||
num_classes: Number of semantic classes to predict | |||
loss: Name of the loss function | |||
ignore_zeros: Whether to ignore the "0" class value in the loss and metrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not call it "ignore_index" and make it an int that defaults to zero? That gives the most flexibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because nn.CrossEntropyLoss uses -1000 as a None value so there is not an easy way to not ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand. I'm saying to rename the ignore_zeros
parameter to ignore_index
and have it accept an int, not to remove the parameter entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but say I don't want to ignore anything -- I can't pass None because nn.CrossEntropyLoss behaves differently than everything else that accepts ignore_index
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
@adamjstewart how do you want to test the plot methods? |
I would personally be fine with a simple test that just runs |
This reverts commit 502a378.
Relevant bit for later
|
I removed the trainer and reset the test data as I can't be bothered to jump through all these hoops. Note, this dataset does need augmentations as training will overfit to the train split (the test set is sampled from a different geographic location and those inputs are likely out-of-distribution to the training data). The above comment is the relevant part to add to |
* Adding ETCI2021 datamodule and trainer * Fixing confs * Update torchgeo/datasets/etci2021.py Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com> * Update torchgeo/trainers/etci2021.py Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com> * Update conf.py * Reverting trainer init * plot method * Update ETCI2021 test data * Test trainer * Fix train.py * Fix test * Adding matplotlib intersphinx * Remove ignore * Revert "Update ETCI2021 test data" This reverts commit 502a378. * Remove stuff * Reset * Add plot tests * Unzipping test data * Test datamodule * Add datamodule to docs Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Relevant links: