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 StackOverflowError coming from MOI.Utilities.operate #6

Merged
merged 20 commits into from
Sep 22, 2023

Conversation

bachdavi
Copy link
Member

No description provided.

src/SolverAPI.jl Outdated Show resolved Hide resolved
test/solve_tests.jl Outdated Show resolved Hide resolved
src/SolverAPI.jl Outdated Show resolved Hide resolved
src/SolverAPI.jl Outdated Show resolved Hide resolved
src/SolverAPI.jl Outdated Show resolved Hide resolved
@chriscoey chriscoey self-requested a review September 21, 2023 16:03
@chriscoey
Copy link
Member

as discussed here - let's attempt to fix this upstream in MOI rather than adding to the complexity of SolverAPI

@chriscoey chriscoey marked this pull request as draft September 21, 2023 16:41
@chriscoey
Copy link
Member

The MOI fix jump-dev/MathOptInterface.jl#2285 unblocks us for the test case added here, so lets get the test case in after MOI is tagged and leave this PR up here as DNMY. Not super high priority but when we become more concerned with performance, I think we could attempt an even larger change here that builds the SAF/SQF/SNF directly bottom-up from the JSON3 data structure, rather than going through the SNF intermediate. Because right now we do JSON->SNF with json_to_snf then SNF->SAF/SQF/SNF (whichever most canonical form we can convert to) with nl_to_aff_or_quad.

@bachdavi
Copy link
Member Author

@chriscoey Sure we can wait for the upstream fix. I'm leaving this PR open in case we want to improve the performance of nl_aff_or_quad. I've taken the large-model from another one of your PRs and the stack-based version is around 3x faster and consumes 2x less memory (both running with the MOI fix). It makes a lot of sense to try to directly go from JSON to the respective function instead of this intermediate.

@chriscoey
Copy link
Member

Thanks @bachdavi. Let's keep working on this PR to try to extend it to go directly from JSON. Hopefully that will improve the efficiency even further.

@chriscoey chriscoey marked this pull request as ready for review September 22, 2023 19:06
@chriscoey
Copy link
Member

Since the upstream MOI fix is controversial, I decided to get this PR in rather than wait. We can further improve the performance in future PRs.

I split out the JSON-to-MOI functions into a separate file to improve clarity.

@chriscoey chriscoey merged commit 5e92c25 into main Sep 22, 2023
3 checks passed
@chriscoey chriscoey deleted the dba-fix-stackoverflow branch September 22, 2023 19:12
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.

3 participants