-
Notifications
You must be signed in to change notification settings - Fork 379
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
BigEarthNet Trainers #211
BigEarthNet Trainers #211
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.
Doesn't this trainer need to be added to train.py
?
c4d243e
to
d6e548b
Compare
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 still haven't heard a good reason why we can't have a single ClassificationTask
and MultiLabelClassificationTask
without all of these subclasses.
I refactored test_tasks to use a dummy dataset and datamodule so we can test for varying number of channels but avoid having to repeat tests for bigearthnet and so2sat datamodule specific args. Let me know if this works. |
You are right thought that there is not a good case for making a separate task for each classification dataset. We can definitely remove them if that's the route you want to take. |
DataLoader.__module__ = "torch.utils.data" | ||
|
||
|
||
class BigEarthNetClassificationTask(MultiLabelClassificationTask): |
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 think getting rid of this class is the last remaining thing to do before this can be merged.
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.
Personally I would refactor ClassificationTask
and MultiLabelClassificationTask
first in separate PRs to keep this PR from getting any bigger.
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.
We should merge this because I still need to refactor the num classes as an input arg before I can remove it. If I do that then I will need to remove/refactor all other classification tasks as well.
* add additional bigearthnet test data for train/val/test split * update bigearthnet dataset length test * add MultiLabelClassificationTask * add BigEarthNet trainer and datamodule * add bigearthnet and multilabelclassificationtask tests * mypy and format * add estimated band min/max values for normalization * softmax outputs to correctly compute metrics * update min/max stats for 100k samples * organize imports in torchgeo.trainers.__init__.py * clean up fixtures in test_tasks.py * added bigearthnet to train.py * format * move fixtures into class methods * consolidate bigearthnet fixtures * refactor tasks tests * add scope=class * style/mypy fixes * mypy fixes
torchgeo.trainers.BigEarthNetClassificationTask
andtorchgeo.trainers.BigEarthNetDataModule
torchgeo.trainers.tasks.MultiLabelClassficationTask
. This differs fromClassificationTask
by usingtorch.nn.BCEWithLogitsLoss
and modifies the metrics to handle multilabel outputsTODO: