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

Rename max_iters to cosine_schedule_period_iters #300

Merged
merged 16 commits into from
Feb 22, 2024
Merged

Conversation

bittremieux
Copy link
Collaborator

Fixes #242.

cosine_schedule_period_iters better reflects what this config option does, and has correspondingly been renamed.

The config loader checks whether max_iters is specified and automatically remaps it to cosine_schedule_period_iters, while warning the user about this renaming.

@bittremieux bittremieux linked an issue Feb 20, 2024 that may be closed by this pull request
Copy link

codecov bot commented Feb 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (83a1ce4) 89.64% compared to head (0c0ccaa) 89.77%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #300      +/-   ##
==========================================
+ Coverage   89.64%   89.77%   +0.13%     
==========================================
  Files          12       12              
  Lines         917      929      +12     
==========================================
+ Hits          822      834      +12     
  Misses         95       95              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@melihyilmaz melihyilmaz left a comment

Choose a reason for hiding this comment

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

When I ran the branch locally to fine-tune v4.0 weights, I got the error below:

optimizer = torch.optim.Adam(self.parameters(), **self.opt_kwargs)
TypeError: Adam.init() got an unexpected keyword argument 'max_iters'

Since max_iters is still in the ckpt file, we pick it up as a part of kwargs. I think we can manually delete it from the current ckpts but I'm not sure if that'd break anything @bittremieux.

@bittremieux
Copy link
Collaborator Author

Ideally we fix it in the code as well. Otherwise other people who try fine-tuning will have the same problem.

I added a unit test to check this and added a fix to remove unrecognized hyperparameters during model loading. This seems to work, but I find it a bit less elegant than the previous fix, because now we're changing config options both in Config and in Spec2Pep. Do you see any alternative solutions @melihyilmaz @wfondrie?

Copy link
Collaborator

@melihyilmaz melihyilmaz left a comment

Choose a reason for hiding this comment

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

Everything works for me now locally but I can't think of a more elegant alternative.

@bittremieux bittremieux merged commit a6cb0ce into dev Feb 22, 2024
6 checks passed
@bittremieux bittremieux deleted the rename_max_iters branch February 22, 2024 09:13
bittremieux added a commit that referenced this pull request May 14, 2024
* Remove `train_from_scratch` config option (#275)

Instead of having to specify `train_from_scratch` in the config file, training will proceed from an existing model weights file if this is given as an argument to `casanovo train`.

Fixes #263.

* Stabilize torch.topk() behavior (#290)

* Add epsilon to index zero

* Fix typo

* Use base PyTorch for repeating along the vocabulary size

* Combine masking steps

* Lint with updated black version

* Lint test files

* Add topk unit test

* Fix lint

* Add fixme comment for future

* Update changelog

* Generate new screengrabs with rich-codex

---------

Co-authored-by: Wout Bittremieux <wout@bittremieux.be>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Rename max_iters to cosine_schedule_period_iters (#300)

* Rename max_iters to cosine_schedule_period_iters

* Add deprecated config option unit test

* Fix missed rename

* Proper linting

* Remove unnecessary logging

* Test that checkpoints with deprecated config options can be loaded

* Minor change

* Add test for fine-tuning with deprecated config options

* Remove deprecated hyperparameters during model loading

* Include deprecated hyperparameter warning

* Test whether the warning is issued

* Verify that the deprecated option is removed

* Fix comments

* Avoid defining deprecated options twice

* Remap previous renamed config option `every_n_train_steps`

* Update changelog

---------

Co-authored-by: melihyilmaz <yilmazmelih97@gmail.com>

* Add FAQ entry about antibody sequencing

* Don't crash when multiple beams have identical peptide scores (#306)

* Test different beams with identical scores

* Randomly break ties for beams with identical peptide score

* Update changelog

* Don't remove unit test

* Allow csv to handle all newlines (#316)

* Add 9-species model weights link to FAQ (#303)

* Add model weights link

* Generate new screengrabs with rich-codex

* Clarify that these weights should only be used for benchmarking

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Wout Bittremieux <wout@bittremieux.be>

* Add FAQ entry about antibody sequencing (#304)

* Add FAQ entry about antibody sequencing

* Generate new screengrabs with rich-codex

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Melih Yilmaz <32707537+melihyilmaz@users.noreply.github.com>

* Allow csv to handle all newlines

The `csv` module tries to handle newlines itself. On Windows, this leads to line endings of `\r\r\n` instead of `\r\n`.

Setting `newline=''` produces the intended output on both platforms.

* Update CHANGELOG.md

* Fix linting issue

* Delete docs/images/help.svg

---------

Co-authored-by: Melih Yilmaz <32707537+melihyilmaz@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Wout Bittremieux <wout@bittremieux.be>
Co-authored-by: William Stafford Noble <wnoble@uw.edu>
Co-authored-by: Wout Bittremieux <bittremieux@users.noreply.github.com>

* Don't test on macOS versions with MPS (#327)

* Prepare for release v4.2.0

* Update CHANGELOG.md (#332)

---------

Co-authored-by: Melih Yilmaz <32707537+melihyilmaz@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: melihyilmaz <yilmazmelih97@gmail.com>
Co-authored-by: wsnoble <wnoble@uw.edu>
Co-authored-by: Joshua Klein <mobiusklein@gmail.com>
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.

Make the learning rate scheduler more flexible
2 participants