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 Sympiler Wrapper #34

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

Added Sympiler Wrapper #34

wants to merge 14 commits into from

Conversation

ryansynk
Copy link
Collaborator

No description provided.

@ryansynk ryansynk linked an issue Jun 22, 2023 that may be closed by this pull request
src/polysolve/LinearSolverSympiler.hpp Outdated Show resolved Hide resolved
src/polysolve/LinearSolverSympiler.hpp Outdated Show resolved Hide resolved
src/polysolve/LinearSolverSympiler.cpp Outdated Show resolved Hide resolved
src/polysolve/LinearSolverSympiler.cpp Outdated Show resolved Hide resolved
src/polysolve/LinearSolverSympiler.cpp Show resolved Hide resolved
#ifdef POLYSOLVE_WITH_SYMPILER

#include <polysolve/LinearSolver.hpp>
#include <sympiler/parsy/cholesky_solver.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this header to the .cpp. This will require moving the definition of the constructor/destructor to the .cpp of this file as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this possible given the member variables of type sym_lib::parsy::CSC and sym_lib::parsy::SolverSettings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took a crack at it -- let me know how it looks

Copy link

@qnzhou qnzhou left a comment

Choose a reason for hiding this comment

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

I just have a few minor comments. But it looks good to me.


std::unique_ptr<sym_lib::parsy::SolverSettings> m_solver;
std::unique_ptr<sym_lib::parsy::CSC> m_A_csc;
polysolve::StiffnessMatrix m_A_copy;
Copy link

Choose a reason for hiding this comment

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

I am curious if we really need to store a copy of the matrix. Is this done in other solvers as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure about other solvers, but itseems like sympiler needs a SolverSettings object, which takes a pointer directly to the values and indices of a matrix. The pointers aren't const, so I don't think there's a guarantee that the solver object won't mess with the matrix data? That's why I've been doing the copy

Comment on lines +51 to +52
int solver_mode = 0; // 0 is normal solve, 1 is row/col addition
int factorize_status = -1;
Copy link

Choose a reason for hiding this comment

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

@jdumas @teseoch Is there a convention for naming member variables we should follow in polysolve? I see m_ prefix for some member variables and _ postfix for other member variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for using m_ prefix for member variables consistently.

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.

add sympiler wrapper
3 participants