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

Nalu-Wind: Add framework for preset solvers. #1096

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

neilmatula
Copy link
Contributor

No description provided.

Copy link
Contributor

@alanw0 alanw0 left a comment

Choose a reason for hiding this comment

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

I like the idea of having solver presets and configurations, but I don't like putting these new classes and the yaml include into the existing LinearSolvers.h header. Can't they go in a new header (or multiple new headers)?
Generally we've regretted jamming more stuff into existing headers. We generally benefit from keeping things as separated/modularized as feasible.

@neilmatula
Copy link
Contributor Author

Yep, we can totally put this in a new header file. I was considering that as a followup commit anyway.

Copy link
Contributor

@psakievich psakievich left a comment

Choose a reason for hiding this comment

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

These format issues were because a PR was pushed without passing the format CI. Will you please revert the format changes and then rebase? That will remove the superfluous changes due to clang-format that aren't part of this PR.

private:
};

class PresetSolverBase
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall this is fine. It is a little more involved than I was imagining. I think it is kind of hard to read the solver settings this way.

I was thinking we could just embed the yaml in a header using string literals, and embed the xmls in another header using string literals. For an example in nalu-wind check here:

const char* actuatorParameters = R"act(actuator:
search_target_part: dummy
search_method: stk_kdtree
type: ActLineSimpleNGP
n_simpleblades: 1
Blade0:
fllt_correction: yes
num_force_pts_blade: 5
epsilon_min: [3.0, 3.0, 3.0]
epsilon_chord: [0.25, 0.25, 0.25]
p1: [0, -4, 0]
p2: [0, 4, 0]
p1_zero_alpha_dir: [1, 0, 0]
chord_table: [1.0]
twist_table: [0.0]
aoa_table: [-180, 0, 180]
cl_table: [2, 2, 2]
cd_table: [1.2])act";

Then we can access the presets from a std::map rather than needing to create objects for each set of solver settings. something like

// in someheader.h
const char* scalar_tpetra_yaml = R"eq(  - name: solve_scalar
     type: tpetra
     method: gmres
     preconditioner: sgs
     tolerance: 1e-6
     max_iterations: 75
     kspace: 75
     output_level: 0)eq";
// new files
#include<someheader.h>
std::map<std::string, const char*> presets = {"scalar_tpetra", scalar_tpetra_yaml};

Overall I think this would be less infrastructure. This is my opinion though. I'm curious what the other reviewers think.

Also there might be better formats for saving the solver parameters than xml for hypre and tpetra. Like a teuchos::parameterlist. Not sure what the equivalent is for hypre

@marchdf
Copy link
Contributor

marchdf commented Jul 30, 2024

@neilmatula this feels close to being able to merge? Or maybe no longer necessary?

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.

4 participants