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

Review of the MA27 interface #116

Merged
merged 6 commits into from
Dec 1, 2024
Merged

Review of the MA27 interface #116

merged 6 commits into from
Dec 1, 2024

Conversation

cvanaret
Copy link
Owner

@cvanaret cvanaret commented Dec 1, 2024

Fixed following bugs:

  • ikeep has size 3n, not 3nnz;
  • the member factor that initially contains the matrix entries, then the factors after a call to MA27BD, must be initialized in do_numerical_factorization(). This solves the issue with the byrdpreset on hs015 mentioned in MA27 interface #115.

Cleaned up the factorization function (replaced recursive calls with while loop).

cc @worc4021

@worc4021
Copy link
Contributor

worc4021 commented Dec 1, 2024

Oh dear! Amateur issues! Good spot! Very strange that valgrind didn't notice the size issue.

I noticed that we're now copying the entries of the matrix into the factor array twice. We call save_matrix_to_local_format from do_symbolic_factorization but then somehow left it being overridden with the copy from the matrix. Potentially wasteful to be doing both..?!

@cvanaret
Copy link
Owner Author

cvanaret commented Dec 1, 2024

I noticed that we're now copying the entries of the matrix into the factor array twice. We call save_matrix_to_local_format from do_symbolic_factorization but then somehow left it being overridden with the copy from the matrix. Potentially wasteful to be doing both..?!

Absolutely! It turns out we actually get the sparsity pattern from the ASL function sphsetup (called once at the beginning). I'll refactor so that we can query it, store it in the appropriate sparse format once and for all, then we'll work only with the matrix entries from then on.

@cvanaret cvanaret merged commit 48554eb into main Dec 1, 2024
7 checks passed
@cvanaret cvanaret deleted the ma27 branch December 1, 2024 21:30
@cvanaret cvanaret changed the title (WIP) Review of the MA27 interface Review of the MA27 interface Dec 1, 2024
@worc4021
Copy link
Contributor

worc4021 commented Dec 1, 2024

The while loop is a good idea! However, I would introduce an upper limit on how often it can reallocate memory. When Hessians have illegal values in them e.g. a nan in ipopt you can get the linear solvers run off with your memory - the solvers don't check for this. They hopelessly keep trying to reallocate and refactor and eventually your whole process dies, but the longer the work vectors get the slower each round becomes.

@cvanaret
Copy link
Owner Author

cvanaret commented Dec 2, 2024

Good idea. I would set an upper bound of (say) 5 factorization attempts, and I would check the Hessian values beforehand ;)

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.

2 participants