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

Remove the line for resetting the solver type in slepc eigen solver #3342

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

YaqiWang
Copy link
Contributor

Close #3341.

@YaqiWang YaqiWang force-pushed the solver_type_reset_3341 branch from 5149746 to a3423da Compare July 21, 2022 03:57
@moosebuild
Copy link

Job Coverage on a3423da wanted to post the following:

Coverage

2e35f6 #3342 a3423d
Total Total +/- New
Rate 55.72% 55.72% -0.00% -
Hits 44919 44918 -1 0
Misses 35696 35696 - 0

Diff coverage report

Full coverage report

This comment will be updated on new commits.

@YaqiWang
Copy link
Contributor Author

I can add a new function like clearEPS() and call it instead of calling clear() if you prefer the removal of the line. @jwpeterson and @roystgnr.

@jwpeterson
Copy link
Member

I think the right fix here, as @roystgnr suggested, is to try removing the clear() from the different SlepcEigenSolver solve() APIs. The closest analog I see for this is in PetscLinearSolver where we don't call clear() before the solves and it seems to work OK. Have you tried this?

@roystgnr
Copy link
Member

Digging in to the git log: we only added those clear() calls 3 years ago, in #2160 - but they were added to fix a couple actual problems.

@YaqiWang
Copy link
Contributor Author

Yeah, that is the thing I was afraid of. It is very difficult for me to use the slepc solver without relying on setting commandline options to tune the solver even though I am using the EigenSystem. So I decided to just write a very specific small slepc solver for my purpose. So I lost motivation to push this forward. I respect the design to let clear() make the object return its original state. Maybe create a new function like reinit that can be used by solve functions.

@roystgnr
Copy link
Member

roystgnr commented Aug 5, 2022

Maybe create a new function like reinit that can be used by solve functions.

IMHO this may be the right thing to do medium-term.

In hindsight the right thing to do 3 years ago, as well as in the long term, might have been to require user code to call clear() (or a clear_everything_but_solver_type(), or whatever it actually needed) manually as needed, not force it on every solve() call regardless of need. There are typically good performance reasons for wanting to be able to reuse structures (I'm guessing matrix factorizations are the issue, based on skimming threads like https://lists.mcs.anl.gov/pipermail/petsc-users/2016-November/030870.html ?) created in one solve for another similar solve, even if that policy can be pointless in some circumstances and can outright backfire on dissimilar solves.

The trouble with this is I'm not sure where to put that code at the user level. I guess we could experiment with removing the clear() calls entirely, find the failing tests, and look for the cases that need a manual clear(). And if it turns out that we missed some then we'll be teaching downstream users a valuable lesson about writing enough test coverage?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants