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

Adding RichardsonLinearSolver #87

Merged
merged 5 commits into from
Dec 21, 2024
Merged

Adding RichardsonLinearSolver #87

merged 5 commits into from
Dec 21, 2024

Conversation

shreyas02
Copy link
Contributor

I am adding RichardsonLinearSolver as discussed with @JordiManyer in #86. Tests are yet to be added for this solver.

@shreyas02 shreyas02 changed the title Adding RichadsonLinearSolver Adding RichardsonLinearSolver Dec 18, 2024
@shreyas02 shreyas02 closed this Dec 18, 2024
@shreyas02 shreyas02 reopened this Dec 18, 2024
@JordiManyer JordiManyer self-requested a review December 18, 2024 21:34
Copy link
Member

@JordiManyer JordiManyer left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

As a general comment, I would say the code is riddled with allocations. All allocations need to be moved to the numerical_setup phase, and the solve! method has to be allocation-free. I would use in-place operations instead of so much copying around.

Also:

  • You need tests, both in serial and distributed.
  • You need to summarize changes in NEWS.md
  • You need to add the solver to the documentation, under src/LinearSolvers.md

src/LinearSolvers/RichardsonLinearSolvers.jl Outdated Show resolved Hide resolved
src/LinearSolvers/RichardsonLinearSolvers.jl Outdated Show resolved Hide resolved
src/LinearSolvers/RichardsonLinearSolvers.jl Outdated Show resolved Hide resolved
src/LinearSolvers/RichardsonLinearSolvers.jl Outdated Show resolved Hide resolved
src/LinearSolvers/RichardsonLinearSolvers.jl Outdated Show resolved Hide resolved
src/LinearSolvers/RichardsonLinearSolvers.jl Outdated Show resolved Hide resolved
@JordiManyer
Copy link
Member

@shreyas02 I think that for some reason microsoft removed BLAS from it's CI nodes. It's fixed now, just pull from master and tests should pass.

@shreyas02
Copy link
Contributor Author

shreyas02 commented Dec 20, 2024

Hi @JordiManyer, the CI for Documentation is failing. Any reason why?

I had left a line between the docstring """ and the struct definition. I have removed that. Maybe that works?

@JordiManyer JordiManyer merged commit d4fb231 into gridap:main Dec 21, 2024
2 checks passed
@JordiManyer
Copy link
Member

Thank you for your contribution @shreyas02 !

@shreyas02
Copy link
Contributor Author

Thanks a lot for the help @JordiManyer!

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