Skip to content

Conversation

@PonteIneptique
Copy link
Contributor

See #76

Copy link
Owner

@emanjavacas emanjavacas left a comment

Choose a reason for hiding this comment

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

Hey. The changes to config are cluttering a bit the code. We should group lr-related parameters. I could take care of that. But I am more concerned with the inclusion of this new dependency for the optimizers. CosineAnnealing is already implemented in pytorch, so what's all that additional stuff contributing? We should strive to keep things as simple as possible and only bring in new deps and functionality if it's really needed, that's why I am asking.

@PonteIneptique
Copy link
Contributor Author

The new dependency is mostly here to bring other optimizers. I tested Ranger but there are many more. It did not yield better results though. But it provides space for researching on this front. Does that answer ?
I agree on grouping LR Related parameters, I decided to not add a sub-dictionary because I was not sure which way you'd like to go. :)

@emanjavacas
Copy link
Owner

emanjavacas commented Dec 6, 2020 via email

@PonteIneptique
Copy link
Contributor Author

No problem, there is also nothing urgent here (as everything is touching only the Trainer, not the deployed model).
I am currently running same kind of test on a bigger corpus. Can we decided how how the parameters should be shown until then ?

@PonteIneptique
Copy link
Contributor Author

Closed in favor of #79

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