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

Added the posibility to modify the control paramteres of UMFPACK alg #482

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

PaoloBiolghini
Copy link

@PaoloBiolghini PaoloBiolghini commented Mar 15, 2024

#383
I have added a vector of type Float 64 in the UMFPACKFactorization structure in order to give the possibility to the user to plug in their customized control vector int the UMFPACK algorithm;
This control vector is used in the solve function and is passed to SparseArrays through the lu function.

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

For a deeper understing on the control vector i sugest to read subsection 5.10 at page 23 in the following pdf
https://github.com/DrTimothyAldenDavis/SuiteSparse/blob/dev/UMFPACK/Doc/UMFPACK_UserGuide.pdf

@ChrisRackauckas
Copy link
Member

Needs docs.

@@ -739,6 +739,7 @@ patterns with “more structure”.
Base.@kwdef struct UMFPACKFactorization <: AbstractFactorization
reuse_symbolic::Bool = true
check_pattern::Bool = true # Check factorization re-use
control::Vector{Float64}=SparseArrays.UMFPACK.get_umfpack_control(Float64,Int64) # Control Parameters of UMFPACK
Copy link
Member

Choose a reason for hiding this comment

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

This will heap allocate each time in the default algorithm. Instead, make a const of the default, set the default here to nothing, and then if nothing use the const global.

Copy link

codecov bot commented Mar 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 22.49%. Comparing base (c08f2e9) to head (64d80d1).

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #482       +/-   ##
===========================================
- Coverage   64.22%   22.49%   -41.74%     
===========================================
  Files          28       28               
  Lines        2200     2165       -35     
===========================================
- Hits         1413      487      -926     
- Misses        787     1678      +891     

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

…qual to nothing; in the solver if no control has been given use the default values
@@ -736,9 +736,12 @@ patterns with “more structure”.
`A` has the same sparsity pattern as the previous `A`. If this algorithm is to
be used in a context where that assumption does not hold, set `reuse_symbolic=false`.
"""
const default_control::Array{FLoat64}=[1.0, 0.2, 0.2, 0.1, 32.0, 0.0, 0.7, 0.0, 1.0, 0.3, 1.0, 1.0, 0.9, 0.0, 10.0, 0.001, 1.0, 0.5, 0.0, 1.0]
Copy link
Member

Choose a reason for hiding this comment

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

This is blocking the docstring above from going to the right thing.

preserved. Defaults to true.
* `check_pattern`: Whether the sparsity pattern is checked for changes to allow for symbolic factorization caching.
The default is true.
* `control`: A control vector for more options to pass to UMFPACK. See the UMFPACK documentation for more details.
Copy link
Member

Choose a reason for hiding this comment

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

We should fill in the details here, since it has been unchanged for 20 years. Is there something simple that can be pasted, quoted, or linked to?

Copy link
Author

Choose a reason for hiding this comment

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

eventualli we could somehow insert the table present in the doc of UFMPACK
image

Copy link
Author

Choose a reason for hiding this comment

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

do u think would be appropriate?

@ChrisRackauckas
Copy link
Member

Co-authored-by: Christopher Rackauckas <[email protected]>
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