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

Add wrapper for FieldMatrix and FieldMatrixSolver #1788

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

dennisYatunin
Copy link
Member

This PR adds the InvertibleFieldMatrix struct, which is a simple wrapper for a FieldMatrix and a corresponding FieldMatrixSolver. The wrapper has a constructor that specifies a default FieldMatrixSolverAlgorithm, and it also has methods for similar, ldiv!, and mul!. This should allow us to remove a lot of boilerplate code that is currently needed to set up implicit solvers (see this comment in ClimaTimeSteppers).

  • Code follows the style guidelines OR N/A.
  • Unit tests are included OR N/A.
  • Code is exercised in an integration test OR N/A.
  • Documentation has been added/updated OR N/A.

@dennisYatunin dennisYatunin added the enhancement New feature or request label Jun 7, 2024
@dennisYatunin dennisYatunin force-pushed the dy/field_matrix_wrapper branch from 3227620 to bf04421 Compare June 7, 2024 18:47
@Sbozzolo
Copy link
Member

Sbozzolo commented Jun 7, 2024

Looks good, please fix the tests, and maybe let's make another release so that we can use it in ClimaTimeSteppers.

@dennisYatunin dennisYatunin force-pushed the dy/field_matrix_wrapper branch 2 times, most recently from 7804a14 to ecd27ad Compare June 7, 2024 21:39
@Sbozzolo Sbozzolo force-pushed the dy/field_matrix_wrapper branch from ecd27ad to b4dc488 Compare June 10, 2024 16:36
Copy link
Member

@charleskawczynski charleskawczynski left a comment

Choose a reason for hiding this comment

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

Overall I think that this is an improvement, but I think that some names can be improved. Let's let #1787 get through and rebase this, first.

@Sbozzolo Sbozzolo force-pushed the dy/field_matrix_wrapper branch from b4dc488 to c10bf48 Compare June 10, 2024 17:30
@Sbozzolo Sbozzolo force-pushed the dy/field_matrix_wrapper branch 2 times, most recently from 233eca1 to 1172357 Compare June 10, 2024 21:22
@Sbozzolo Sbozzolo enabled auto-merge June 10, 2024 21:24
@Sbozzolo Sbozzolo force-pushed the dy/field_matrix_wrapper branch from 1172357 to d4ae6ef Compare June 10, 2024 21:25
@Sbozzolo Sbozzolo merged commit 9bb1a73 into main Jun 10, 2024
17 of 19 checks passed
@Sbozzolo Sbozzolo deleted the dy/field_matrix_wrapper branch June 10, 2024 22:43
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.

3 participants