-
Notifications
You must be signed in to change notification settings - Fork 178
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
FEAT: Add finetune_depth parameter #471
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Experiment ResultsExperiment 1: air-passengersDescription:
Results:
Plot:Experiment 2: air-passengersDescription:
Results:
Plot:Experiment 3: electricity-multiple-seriesDescription:
Results:
Plot:Experiment 4: electricity-multiple-seriesDescription:
Results:
Plot:Experiment 5: electricity-multiple-seriesDescription:
Results:
Plot: |
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.
Nice! Few comments, and I think we should include a test in nixtla_client.ipynb
.
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.
LGTM, I removed last couple of mentions of layers.
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.
Please also add a test verifying that as the finetune depth increases the loss is lower in the nbs/docs/tutorials/06_finetuning.ipynb
notebook.
Co-authored-by: José Morales <[email protected]>
Such a test doesn't always work; i.e. it's not always the case that finetuning improves the model. So I'm removing it again.... |
…xtla into feature/finetune_depth
What changed between now and 7bc1b5d? That one has very different results (nb link). This is supposed to be deterministic, isn't it? I'd expect to be able to reproduce the metrics from that commit every time, especially the monotonic part, right now 2 and 3 yield the same result which is highly suspicious. |
Nothing really changed, the issue is that it doesn't hold in general that:
I tweaked the parameters so that the results are not good but monotonic (however as said before, finetuning isn't guaranteed to provide results that are strictly better when increasing the parameters) |
Thanks! So the test would pass now? It'd be great having |
No, because:
so we shouldn't market that view, either. And a test on that is useless too; if it fails, the results might still be better than before. I've updated the example to explain that also (so users see that increasing depth can also worsen performance, and it's a bit of trial and error). The tutorial fails if the parameter isn't passed through correctly, so we're covered there anyways. |
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.
see notebook
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.
LGTM, I only fixed some typos in the latest revision
Add the
finetune_depth
parameter to control how many layers are finetuned.Adjust tutorials and capabilities with new parameter.