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

Fix #111 and maybe minor fixes WIP #113

Merged
merged 17 commits into from
Mar 17, 2024
Merged

Fix #111 and maybe minor fixes WIP #113

merged 17 commits into from
Mar 17, 2024

Conversation

votroto
Copy link
Contributor

@votroto votroto commented Mar 12, 2024

Updating of polynomial variables throughout final_touch meant that polynomial and MOI variables were no longer in sync; this resulted in nonsensical models.

My solution just updates the variables, then takes the sorting approach from _scalar_polynomial in nl_to_polynomial.jl. It is not great, but without more significant changes, some variable-order-preserving hocus-pocus will have to be done anyway.

I took the liberty of suggesting an edit for the / overload, which should now try to avoid creating rational polynomials.

The _to_polynomial for ScalarQuadraticFunction in functions.jl seems redundant after issue #110.

Tests that the variables are preserved should be written, but without more significant changes, that might be tricky. Maybe just a few e2e tests in the meantime? I will write a few for the /, at least.

Fixes #111

@votroto votroto marked this pull request as draft March 12, 2024 13:17
@votroto
Copy link
Contributor Author

votroto commented Mar 12, 2024

After more testing, this does not solve the issue.

@votroto
Copy link
Contributor Author

votroto commented Mar 12, 2024

Ah, it was wrong in the _scalar_polynomial already too.

src/nl_to_polynomial.jl Outdated Show resolved Hide resolved
src/QCQP/MOI_wrapper.jl Outdated Show resolved Hide resolved
@blegat
Copy link
Member

blegat commented Mar 12, 2024

The changes look good, thanks, just missing a few tests. Yes, unit tests calling the internal functions is good.

@votroto
Copy link
Contributor Author

votroto commented Mar 13, 2024

Ok, about ready for review. The tests are so complex we might need tests for the tests, but they were failing before and are passing now, and they do cover a lot of the changes. So... success? Is there a standard way to do randomized/property testing in Julia?

I'd also welcome an explanation on what the type T of the _scalar_polynomial function is, because I can't get anything to compile consistently. That might simplify the tests a tiny bit.

If this is good enough, I could try proposing a refactoring PR, if such a thing would be welcome.

BTW, Is it expected for dynamic polynomials with rational coefficients to be a little unstable?

@votroto votroto marked this pull request as ready for review March 14, 2024 10:30
@blegat
Copy link
Member

blegat commented Mar 14, 2024

Need to fix format

src/nl_to_polynomial.jl Outdated Show resolved Hide resolved
test/qcqp_extra.jl Outdated Show resolved Hide resolved
test/qcqp_extra.jl Outdated Show resolved Hide resolved
test/qcqp_extra.jl Outdated Show resolved Hide resolved
test/qcqp_extra.jl Outdated Show resolved Hide resolved
test/qcqp_extra.jl Outdated Show resolved Hide resolved
test/qcqp_extra.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member

odow commented Mar 15, 2024

@blegat are the QCQP tests even run?

test/qcqp_extra.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member

odow commented Mar 17, 2024

I've dropped the change to a / b. I'll re-add it in a separate PR.

@odow odow merged commit faf3389 into jump-dev:master Mar 17, 2024
7 checks passed
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.

PolyJuMP makes a feasible model infeasible
3 participants