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

lusol factor extraction #178

Merged
merged 15 commits into from
Sep 12, 2024
Merged

lusol factor extraction #178

merged 15 commits into from
Sep 12, 2024

Conversation

superwhiskers
Copy link
Collaborator

@superwhiskers superwhiskers commented Jul 19, 2024

Resolves #159.

what remains to be done:

  • rebase on develop from lusol-integration-tests
  • use already existing L_ and U_ member variables instead of allocating new ones
  • constrain size by nsing
  • add tests for factor extraction

@superwhiskers superwhiskers linked an issue Jul 22, 2024 that may be closed by this pull request
@superwhiskers superwhiskers marked this pull request as ready for review July 23, 2024 13:29
@superwhiskers
Copy link
Collaborator Author

wasn't able to solve it with a rebase. i ended up making a new branch locally and cherry-picking the commits from the old one

@superwhiskers superwhiskers added the enhancement New feature or request label Jul 23, 2024
@superwhiskers
Copy link
Collaborator Author

this should be ready to merge now

Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

The code needs to be better commented. There are some nontrivial indexing patterns and there need to be comments explaining what is being done in main loops in L and U factor getters. Also, it is not clear what each parameter from LUSOL parameter array is. This should be explained in comments in the code.

resolve/LinSolverDirectLUSOL.hpp Outdated Show resolved Hide resolved
tests/unit/matrix/LUSOLTests.hpp Outdated Show resolved Hide resolved
resolve/LinSolverDirectLUSOL.cpp Outdated Show resolved Hide resolved
resolve/LinSolverDirectLUSOL.cpp Outdated Show resolved Hide resolved
resolve/LinSolverDirectLUSOL.cpp Outdated Show resolved Hide resolved
resolve/LinSolverDirectLUSOL.cpp Outdated Show resolved Hide resolved
resolve/LinSolverDirectLUSOL.cpp Outdated Show resolved Hide resolved
resolve/LinSolverDirectLUSOL.cpp Outdated Show resolved Hide resolved
@pelesh
Copy link
Collaborator

pelesh commented Jul 29, 2024

This is very good work. Apart from commenting code better, this should be ready to merge.

resolve/LinSolverDirectLUSOL.cpp Show resolved Hide resolved
}

// handle rectangular l factors correctly
for (index_type column = 0; column < diagonal_bound; column++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this supposed to sort columns through bubble sort type algorithm?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this loop just adds ones to the diagonal of L.

Copy link
Collaborator Author

@superwhiskers superwhiskers Sep 12, 2024

Choose a reason for hiding this comment

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

correct. that's exactly what it does

Copy link
Collaborator Author

@superwhiskers superwhiskers Sep 13, 2024

Choose a reason for hiding this comment

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

to elaborate, this loop corrects the column pointer of each column segment by the offsets necessary to accommodate an extra element in itself and all previous columns. it then inserts the one at the end of the column's allocated space, since it

  1. will always be the last element in a sorted column
  2. would add extra computational overhead if it were placed at the beginning (since it would incur an extra n - 1 swaps or something like that) to get the column sorted

sorting of the elements lusol writes to the l factor is done in the next loop, and that is done with a kind of insertion sort, as the comment above it states

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for helpful additional explanations, @superwhiskers.

resolve/LinSolverDirectLUSOL.cpp Show resolved Hide resolved
resolve/LinSolverDirectLUSOL.cpp Show resolved Hide resolved
@pelesh pelesh merged commit e38e2b8 into develop Sep 12, 2024
4 checks passed
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.

Add functions to extract L and U factors from LUSOL
3 participants