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

Test conic forms for constant values #617

Merged
merged 7 commits into from
May 5, 2024
Merged

Test conic forms for constant values #617

merged 7 commits into from
May 5, 2024

Conversation

ericphanson
Copy link
Collaborator

@ericphanson ericphanson commented May 4, 2024

Addresses #616 in the real case only. In the complex case unfortunately I found many issues, too many to address in this PR. Some would be addressed similarly to the change for new_conic_form!(context::Context{T}, e::RelativeEntropyAtom), where GenericConstraint nicely handles the constant case, but direct MOI_add_constraint does not.

(The issues here are not really correctness bugs, but rather errors one would get if for some reason they had constant values rather than variables in some atoms, such as if you fix!'d a variable).

Copy link

codecov bot commented May 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.79%. Comparing base (ac382bb) to head (2088bb1).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #617      +/-   ##
==========================================
+ Coverage   97.75%   97.79%   +0.04%     
==========================================
  Files          88       88              
  Lines        5073     5075       +2     
==========================================
+ Hits         4959     4963       +4     
+ Misses        114      112       -2     

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

test/test_atoms.jl Outdated Show resolved Hide resolved
@odow odow force-pushed the eph/test-constants branch from 207431b to bfffeab Compare May 5, 2024 21:01
@odow
Copy link
Member

odow commented May 5, 2024

Okay. I've simplified the test handling, but now it requires #623

@odow odow force-pushed the eph/test-constants branch from f3ad47c to 1889d7d Compare May 5, 2024 21:57
@odow odow changed the base branch from master to od/relative-entropy-cone May 5, 2024 21:57
@odow
Copy link
Member

odow commented May 5, 2024

Urgh. I borked that. Let's just wait and I'll rebase on master.

Base automatically changed from od/relative-entropy-cone to master May 5, 2024 22:25
@odow odow force-pushed the eph/test-constants branch from 1889d7d to d697095 Compare May 5, 2024 22:26
context = Convex.Context{value_type}(MOI.Utilities.Model{value_type})
atom = _to_constant(build_fn(context))
if Convex.iscomplex(atom) || any(Convex.iscomplex, atom.children)
return
Copy link
Member

Choose a reason for hiding this comment

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

I'll fix this in a separate PR.

@odow odow merged commit ac92d4a into master May 5, 2024
10 checks passed
@odow odow deleted the eph/test-constants branch May 5, 2024 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants