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

Refactor evaluation loop for empty frames #858

Merged
merged 23 commits into from
Jan 8, 2025

Conversation

bw4sz
Copy link
Collaborator

@bw4sz bw4sz commented Dec 17, 2024

This PR aims to standardize, document and better test those situations in which 1) the model doesn't make predictions, but there are ground truth to evaluate and 2) where there are no ground truth in validation, but the model makes predictions. I introduce a new evaluation metric 'empty frame accuracy' that will be useful for many users, I added tests and docs. During this, I found that the test that was called validation_step wasn't fully testing validation_step, but rather trainer.validate, which is related, but not the same. I added a proper validation step test, which required silencing the loggers, due to pytorch lightning being overeager in throwing errors if self.validation_step is run outside of trainer.validate, which is only needed for testing situations.

@bw4sz bw4sz requested a review from henrykironde December 24, 2024 01:03
@bw4sz bw4sz changed the title [WIP] Refactor evaluation loop for empty frames Refactor evaluation loop for empty frames Dec 24, 2024
@bw4sz
Copy link
Collaborator Author

bw4sz commented Jan 4, 2025

Tensorboard is no longer a required module in pytorch lightning, so need to be explictly installed. I am updating the tests since this got caught more as a side-effect and not anything to do with tensorboard.

@bw4sz
Copy link
Collaborator Author

bw4sz commented Jan 6, 2025

@henrykironde this is ready, sorry for the delay with all the docstring commits, my sphinx version was outdated.

Copy link
Contributor

@henrykironde henrykironde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bw4sz, this looks good! One thought: could we handle the try blocks more specifically? They feel a bit broad as they are.

src/deepforest/main.py Show resolved Hide resolved
src/deepforest/main.py Show resolved Hide resolved
@henrykironde henrykironde added the Awaiting author contribution Waiting on the issue author to do something before proceeding label Jan 7, 2025
@bw4sz bw4sz force-pushed the refactor_validation_epoch_end_empty_frames branch from 692bcc7 to 68a69d7 Compare January 7, 2025 19:49
@github-actions github-actions bot removed the Awaiting author contribution Waiting on the issue author to do something before proceeding label Jan 7, 2025
@bw4sz
Copy link
Collaborator Author

bw4sz commented Jan 7, 2025

I rebased, I noticed that somehow the batch tests were commented out, I'll make a separate PR, that is unrelated.

@henrykironde henrykironde merged commit bc5cad7 into main Jan 8, 2025
4 checks passed
@henrykironde henrykironde deleted the refactor_validation_epoch_end_empty_frames branch January 8, 2025 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants