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

Print initial validation loss + final validation loss #1228

Merged
merged 11 commits into from
Apr 29, 2024
Merged

Print initial validation loss + final validation loss #1228

merged 11 commits into from
Apr 29, 2024

Conversation

rasbt
Copy link
Collaborator

@rasbt rasbt commented Apr 1, 2024

EDIT: After Adrian already added the final validation, I greatly simplified this PR. So this only adds the initial validation. Since there was not that much enthusiasm from others, I made it optional and disabled it by default. I think it's a super useful feature to be able to know what the initial val_loss of the model is so that one can compare it to the final one and be able to tell if/how much things improved after training.

@rasbt rasbt added the enhancement New feature or request label Apr 1, 2024
litgpt/finetune/adapter.py Outdated Show resolved Hide resolved
litgpt/finetune/adapter.py Outdated Show resolved Hide resolved
litgpt/finetune/adapter.py Outdated Show resolved Hide resolved
litgpt/finetune/adapter.py Outdated Show resolved Hide resolved
litgpt/finetune/adapter.py Outdated Show resolved Hide resolved
@rasbt rasbt changed the title Print initial validation loss + final training and validation loss Print initial validation loss + final validation loss Apr 25, 2024
@rasbt
Copy link
Collaborator Author

rasbt commented Apr 25, 2024

I greatly simplified this @carmocca . Let me know if that's ok now. It should introduce any changes to the defaults.

Copy link
Contributor

@carmocca carmocca left a comment

Choose a reason for hiding this comment

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

LGTM minus the adapter copy-paste accidents

litgpt/finetune/adapter.py Outdated Show resolved Hide resolved
litgpt/finetune/lora.py Outdated Show resolved Hide resolved
@rasbt
Copy link
Collaborator Author

rasbt commented Apr 26, 2024

Is it really needed to add an argument for this? Why not just use the initial sanity check as the initial validation loss?

I originally had that but others found it was misleading because it's calculated only on 2 batches and is not reliable. I then replaced it with the full loss calculation but then this was too slow. So the new argument is a compromise so that I can enable it for me but it doesn't slow it down for everyone.

@rasbt rasbt enabled auto-merge (squash) April 26, 2024 11:28
@rasbt rasbt merged commit fe1374c into main Apr 29, 2024
9 checks passed
@rasbt rasbt deleted the print-loss branch April 29, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants