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

Introduced an entry change stopping criterion and generalise the gradient norm SC. #345

Merged
merged 12 commits into from
Jan 18, 2024

Conversation

kellertuer
Copy link
Member

@kellertuer kellertuer commented Jan 16, 2024

This PR

  • introduces a StopWhenEntryChangeLess stopping criterion
  • generalises the StopWhenGradientNormLess to accept arbitrary norm functions. Can you check, @mateuszbaran, whether that includes your InfNorm one? I think this way it is much nicer to introduce different Norm-possibilities.
  • the entry change should be used in the PSO to check the population check (instead of “misusing” the iterate one)
  • needs test coverage after clarification of functionality.

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (86a2866) 99.78% compared to head (747efe6) 99.62%.

❗ Current head 747efe6 differs from pull request most recent head f4e830b. Consider uploading reports for the commit f4e830b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #345      +/-   ##
==========================================
- Coverage   99.78%   99.62%   -0.16%     
==========================================
  Files          69       69              
  Lines        6380     6398      +18     
==========================================
+ Hits         6366     6374       +8     
- Misses         14       24      +10     

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

@kellertuer kellertuer added the Ready-for-Review A label for pull requests that are feature-ready label Jan 17, 2024
@kellertuer
Copy link
Member Author

The decrease per patch is just the 10 lines of Strong Wolfe again that depending on the randomness in the tests sometimes are not reached. So this is ready to go.

Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

A few minor comments.

src/plans/stopping_criterion.jl Outdated Show resolved Hide resolved
src/plans/stopping_criterion.jl Outdated Show resolved Hide resolved
src/plans/stopping_criterion.jl Outdated Show resolved Hide resolved
src/solvers/particle_swarm.jl Show resolved Hide resolved
@mateuszbaran
Copy link
Member

BTW, yes, this is enough to represent the inf norm criterion 👍

@kellertuer
Copy link
Member Author

BTW, yes, this is enough to represent the inf norm criterion 👍

Nice! I had hoped so – and feel this is nicer than coming up with a new criterion (which might mean coming up with new crieria for other norms later as well, and then it gets too crowded). Just took me a while to see this relatively simple solution.

kellertuer and others added 2 commits January 17, 2024 18:52
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@kellertuer
Copy link
Member Author

HM I just saw that you had one fix to just compute some norm only once, but it seems to have broken formatting and tests. Will check this only after my lecture and some meetings somewhen in the late afternoon.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@mateuszbaran
Copy link
Member

I've fixed tests and formatting.

@kellertuer
Copy link
Member Author

Great thanks. Is the discussion above clarified and with that this PR ready to merge?

Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

Yes, everything is fine.

@kellertuer kellertuer merged commit b9858c2 into master Jan 18, 2024
12 checks passed
@kellertuer kellertuer deleted the kellertuer/new-stopping-criteria branch May 4, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-for-Review A label for pull requests that are feature-ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants