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

Notebook for Learning Rate Scheduler #1074

Conversation

ParagEkbote
Copy link
Contributor

@ParagEkbote ParagEkbote commented Nov 28, 2024

As described in #275, I've created a notebook that shows the visualization of the learning rate schedulers using the custom scheduler callback method in skorch.

Please let me know if any improvements are needed and I will make the necessary changes.

Fixes #275

cc: @BenjaminBossan

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ParagEkbote ParagEkbote changed the title Add Notebook for Learning Rate Scheduler Notebook for Learning Rate Scheduler Nov 28, 2024
Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR. I haven't done an in-depth review yet but the first impression is already quite good.

Before proceeding, I have a few points, could you please take a look?

  1. Right now, the notebook starts with pip install .... We typically have a check if the notebook runs in a colab environment and only run the installs there (also, with a link to open the notebook in colab). E.g. check out this notebook. Could you please copy the approach?
  2. Right now, you apply the StandardScaler directly on the data. How about creating an sklearn Pipeline with the StandardScaler as the first step and the neural net as the 2nd? That would be a bit cleaner IMO.
  3. You can remove the if __name__ == "__main__": part.
  4. Regarding the visualization, we have a simulate method in LRScheduler. Maybe that can be used instead of having to implement LRHistoryCallback.
  5. Could you please add an entry to the notebook/README.md and to the CHANGES.md?

@ParagEkbote
Copy link
Contributor Author

ParagEkbote commented Dec 2, 2024

I have modified the notebook, and markdown files as per the suggested changes. Here's a summary:

  1. Completed.

  2. Completed.

  3. Completed.

  4. Completed.

  5. I have added placeholder links in the notebooks. When this PR gets merged, I will submit a follow-up PR since the notebook needs to be present in the repo before I can add the links to the markdown files.

Could you please review the changes?

@BenjaminBossan

Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the updates. I have added some comments, please take a look.

notebooks/Learning_Rate_Scheduler.ipynb Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
notebooks/Learning_Rate_Scheduler.ipynb Outdated Show resolved Hide resolved
notebooks/Learning_Rate_Scheduler.ipynb Outdated Show resolved Hide resolved
notebooks/Learning_Rate_Scheduler.ipynb Outdated Show resolved Hide resolved
@ParagEkbote
Copy link
Contributor Author

ParagEkbote commented Dec 5, 2024

Could you please review the updated changes?

@BenjaminBossan

@ParagEkbote
Copy link
Contributor Author

Could you please review the updated changes?

@BenjaminBossan

Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I reviewed the PR again and added a few comments. One change is blocked by a new PR, so I'd say let's wait for that PR first. Please take a look.

notebooks/Learning_Rate_Scheduler.ipynb Outdated Show resolved Hide resolved
notebooks/Learning_Rate_Scheduler.ipynb Outdated Show resolved Hide resolved
notebooks/Learning_Rate_Scheduler.ipynb Outdated Show resolved Hide resolved
notebooks/README.md Outdated Show resolved Hide resolved
@ParagEkbote
Copy link
Contributor Author

I have addressed the mentioned comments.

Could you please review the updated changes?

cc: @BenjaminBossan

@ParagEkbote
Copy link
Contributor Author

Could you please review the updated changes?

cc: @BenjaminBossan

Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Thanks so much for the updates. I went through the notebook again and found only a few small things I'd like to be adjusted. Please check my comments.

notebooks/Learning_Rate_Scheduler.ipynb Outdated Show resolved Hide resolved
notebooks/Learning_Rate_Scheduler.ipynb Show resolved Hide resolved
notebooks/Learning_Rate_Scheduler.ipynb Outdated Show resolved Hide resolved
notebooks/Learning_Rate_Scheduler.ipynb Show resolved Hide resolved
@ParagEkbote
Copy link
Contributor Author

I have addressed the mentioned comments.

Could you please review the updated changes?

cc: @BenjaminBossan

Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Thanks for making the final changes, the notebook looks great, well done.

@BenjaminBossan BenjaminBossan merged commit 79ad021 into skorch-dev:master Dec 30, 2024
16 checks passed
@ParagEkbote
Copy link
Contributor Author

Thank you for assigning me the issue and being a patient guide.

Could you please see that #1025 gets closed to declutter?

cc: @BenjaminBossan

@ParagEkbote ParagEkbote deleted the notebook-for-learning-rate-scheduler branch December 30, 2024 15:53
@BenjaminBossan BenjaminBossan mentioned this pull request Dec 30, 2024
@BenjaminBossan
Copy link
Collaborator

Thanks for the reminder!

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.

docs or notebook: better explain learning rate schedulers
2 participants