on_validation_epoch_end() during sanity checking will not run if val-accuracy-interval is greater than max epochs.

[reddykkeerthi]

Fixes #859

As part of the issue investigation, I tested the initial behavior by creating a dataset with annotations and passing it to the Jupyter notebook. I monkey-patched on_validation_epoch_end() to track and print how often full validation runs and at which epochs. For the test, I set val_accuracy_interval = 30 and max_epochs = 5 (i.e., val_accuracy_interval > max_epochs). As per the documentation, full validation should not run in this case. But it is actually running full validation.

Initally sanity checking happens -

Right after the sanity check, we see on_validation_epoch_end() being called and the complete validation check running -

After training on epoch 0, we see on_validation_epoch_end() being called again and the complete validation check running, since current_epoch is 0 (0 % 30 = 0) -

In the final phase, all epochs trigger on_validation_epoch_end(), but the complete validation check runs only twice—once during sanity checking and once after training when current_epoch = 0 -

All epochs have completed, and the final result is shown below:

All epochs have completed, and the final result is shown below. As expected, complete validation runs four times: during sanity checking, and when current_epoch is 0, 2, and 4

Now, I have modified the code to include a condition that checks if val_accuracy_interval is less than or equal to max_epochs. In this test, val_accuracy_interval = 30 and max_epochs = 5 (i.e., val_accuracy_interval > max_epochs), as shown below -

For this case, we can see that on_validation_epoch_end() is being called, but complete validation is not performed.

I have fixed this line in the code and tested the behavior. Please let me know if any further enhancements are needed. Thank you!

[reddykkeerthi]

In addition to the Jupyter notebook testing, I’ve added test_validation_interval_greater_than_epochs in test_main.py to verify that no full validation metrics are logged when val_accuracy_interval (set to 3) is greater than max_epochs (set to 2). The test checks that box_precision, box_recall, and empty_frame_accuracy are not present in logged_metrics, as full validation is skipped in this case.

Note: The case where val_accuracy_interval is less than or equal to max_epochs is already covered by test_evaluate_on_epoch_interval in the same file.

Please review and let me know if anything else is needed. Thanks!

[reddykkeerthi]

While I was working on this, someone else updated the same function and their changes were merged into main. To avoid conflicts and ensure the latest updates are included, I’ve closed this PR and opened a new one #1025 with the same functionality, now based on the updated version of the function.

[ethanwhite]

Thanks @reddykkeerthi. For future reference the best approach to this is to rebase on main and then force push the updated changes. It looks something like this through command line git:

git checkout validate_epoch # make sure you're on your branch
git fetch upstream # get the latest changes
git rebase upstream/main # replay your commits on top of the latest changes
# if there are conflicts, fix them and then `git add`, `git rebase --continue`
git push -f origin validate_epoch # push your update changes, you need the -f since you rewrote the history

It's certainly fine to just create a new branch and open a new PR, but this kind of workflow is worth getting used to as you start contributing more to bigger projects.

[reddykkeerthi]

Thanks @reddykkeerthi. For future reference the best approach to this is to rebase on main and then force push the updated changes. It looks something like this through command line git:

git checkout validate_epoch # make sure you're on your branch
git fetch upstream # get the latest changes
git rebase upstream/main # replay your commits on top of the latest changes
# if there are conflicts, fix them and then `git add`, `git rebase --continue`
git push -f origin validate_epoch # push your update changes, you need the -f since you rewrote the history

It's certainly fine to just create a new branch and open a new PR, but this kind of workflow is worth getting used to as you start contributing more to bigger projects.

Thanks a lot for this! But I actually committed before the changes were made by someone else on the inital function, and those changes were not reflected in the commit I had already made. Now, when I try to push the newer commit with the changed function, it's showing additional file changes I had not made. So, I couldn't find the right and proper way to deal with this — but I'm sure there must be one.