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

Fixed bug in trainer notebook #434

Merged
merged 1 commit into from
Feb 27, 2022
Merged

Fixed bug in trainer notebook #434

merged 1 commit into from
Feb 27, 2022

Conversation

calebrob6
Copy link
Member

No description provided.

@calebrob6 calebrob6 mentioned this pull request Feb 25, 2022
20 tasks
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Feb 25, 2022
@adamjstewart
Copy link
Collaborator

Continuing discussion from #101 with @robmarkcole and @caleborb6:

There is an error in trainers.ipynb. The csv is created with upper case column names (e.g. train_RMSE).

What file actually creates metrics.csv? I wonder when it changed from lowercase to uppercase...

This is one of the few things that isn't tested...

It should be tested on our release branch, right? If not, we should fix that.

@calebrob6
Copy link
Member Author

calebrob6 commented Feb 26, 2022

What file actually creates metrics.csv? I wonder when it changed from lowercase to uppercase...

It is created by the CSV logger in the notebook. The CSV logger uses the name of the metrics in the trainer. We changed Cyclone to use the regression trainer and added a more comprehensive set of metrics to log at some point. This involved renaming "rmse" --> "RMSE".

It should be tested on our release branch, right? If not, we should fix that.

It is uniquely not! Because that notebook involves actually training models, which is not feasible on CPU, we skip it during tests.

@adamjstewart
Copy link
Collaborator

It is uniquely not! Because that notebook involves actually training models, which is not feasible on CPU, we skip it during tests.

Can we train it for a single mini-batch?

@calebrob6
Copy link
Member Author

Whether the trainer works with the dataset is covered by our other tests. The problem here was the logging assumptions (and the specific problem would only show up at the end of an epoch of training). I don't know how to make a good test for this within the notebook.

@adamjstewart
Copy link
Collaborator

Yes, the point of the tutorial tests is not to test TorchGeo, it's to test the tutorials to ensure that they stay up-to-date.

@adamjstewart adamjstewart added this to the 0.2.1 milestone Feb 27, 2022
@adamjstewart adamjstewart merged commit d132c32 into main Feb 27, 2022
@adamjstewart adamjstewart deleted the docs_fix branch February 27, 2022 20:12
adamjstewart pushed a commit that referenced this pull request Mar 19, 2022
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants