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

Add epsilon to PositiveDefinite #72

Merged
merged 10 commits into from
Feb 26, 2024
Merged

Add epsilon to PositiveDefinite #72

merged 10 commits into from
Feb 26, 2024

Conversation

simsurace
Copy link
Member

In contrast to Positive, the current PositiveDefinite type is not guaranteed to stay strictly positive as the unconstrained parameter goes to zero. This PR changes that to make the behavior of PositiveDefinite analogous to Positive, and introduces a new type for the existing behavior. Open questions:

  • What is a good default value for epsilon? Positive uses sqrt(eps(T)), not sure where this is coming from. I picked eps(T) for now as the other one seemed too big.
  • Is this considered breaking and if yes, is this a problem? Personally, I think that although this is technically breaking, this would not be too disruptive. If it is considered too breaking or a breaking release is not desirable, I would be perfectly happy to change naming such that the strict one becomes the one with a new name (e.g. StrictlyPositiveDefinite) and the existing name keeps behavior.

src/ParameterHandling.jl Outdated Show resolved Hide resolved
src/parameters_matrix.jl Outdated Show resolved Hide resolved
src/parameters_matrix.jl Outdated Show resolved Hide resolved
src/parameters_matrix.jl Outdated Show resolved Hide resolved
test/parameters_matrix.jl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Feb 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.43%. Comparing base (902fbe2) to head (0c5a728).

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   97.41%   97.43%   +0.02%     
==========================================
  Files           8        8              
  Lines         232      234       +2     
==========================================
+ Hits          226      228       +2     
  Misses          6        6              

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

simsurace and others added 2 commits February 10, 2024 13:10
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@simsurace simsurace added the enhancement New feature or request label Feb 10, 2024
test/parameters_matrix.jl Outdated Show resolved Hide resolved
@willtebbutt
Copy link
Member

This is a really good point -- I'm kind of surprised we'd not encountered this before. Could we maybe just stick with a single positive definite type, but incorporate your change, rather than having two types? I think basically we're doing positive-definiteness incorrectly at the minute.

What is a good default value for epsilon? Positive uses sqrt(eps(T)), not sure where this is coming from. I picked eps(T) for now as the other one seemed too big.

No idea to be honest. I'm not aware of a way to pick a really good value. eps(T) seems fine -- users just have to make sure to pick it big enough for their problem.

Is this considered breaking and if yes, is this a problem? Personally, I think that although this is technically breaking, this would not be too disruptive. If it is considered too breaking or a breaking release is not desirable, I would be perfectly happy to change naming such that the strict one becomes the one with a new name (e.g. StrictlyPositiveDefinite) and the existing name keeps behavior.

If you're happy with modifying the existing PositiveDefinite type to make it actually positive definite and not adding an additional type, then I'm in favour of just tagging a breaking release.

@simsurace simsurace changed the title RFC: introduce PositiveDefinite with epsilon Add epsilon to PositiveDefinite Feb 26, 2024
src/ParameterHandling.jl Outdated Show resolved Hide resolved
test/parameters_matrix.jl Outdated Show resolved Hide resolved
simsurace and others added 2 commits February 26, 2024 10:30
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
test/parameters_matrix.jl Outdated Show resolved Hide resolved
@simsurace
Copy link
Member Author

Ok, I removed the additional type and bumped the version.

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

Great. Thanks! Please feel free to merge.

@simsurace simsurace merged commit febc192 into master Feb 26, 2024
7 checks passed
@simsurace simsurace deleted the positive-semidefinite branch February 26, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants