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

Merge 2D magnetostatics branch into serac #63

Open
wants to merge 74 commits into
base: serac
Choose a base branch
from

Conversation

tuckerbabcock
Copy link
Member

This merges all the changes I've made to the magnetostatic solver to make it support evaluation on 2D meshes. Includes all the EM outputs needed for a motor problem, with all of them differentiated.

Items of potentially broad interest:

  • RelaxedNewtonSolver: a newton solver that supports various LineSearch methods. Currently only have a backtracking linesearch implemented that fits a quadratic or cubic and calculates the step as the step that minimizes the fit polynomial.

  • Further developed support for reverse and forward mode output and residual sensitivities

  • Maybe more?

… the 3d version, and can actually converge a nonlinear residual
… 3D. Integrator approach follows a more straightforward AD approach than the previous integrator, using hand written AD of the MagneticEnergy integrator
…d 3D. Once it is differentiated, I will rename it to ForceIntegrator and remove the old one. Set it up so that magnetic energy could be computed on a per attribute basis, so you could calculate just the energy in the airgap for example. Now only add the current density integrator to the winding attributes so that it hopefully runs more efficiently
…fully easier to differentiate than ForceIntegrator2
…is differentiated wrt the mesh and the state
…make sure differentiation of the 2D cases works correctly
…nce. Added LineSearch abstract class, with a BacktrackingLineSearch implementation. Added tests for RelaxedNewton using BacktrackingLineSearch on a simple 2D Rosenbrock minimization problem
…n the field named state, and differentiated it wrt the input state and the mesh
…emoved try-catch from AbstractSolvers calcOutput and output sensitivities methods. Previously they would catch and print the what of any exception thrown, now they will not intercept. This is to allow a NotImplementedException to be thrown further up the chain
mach/mach_output.py Show resolved Hide resolved
mach/mach_state.py Show resolved Hide resolved
mach/mach_state.py Show resolved Hide resolved
src/common/mach_residual.hpp Show resolved Hide resolved
Comment on lines 59 to +62
double closed_double = 1.0;
setValueFromInputs(inputs, "closed-loop", closed_double);
if (fabs(closed_double) < numeric_limits<double>::epsilon())
{
residual.closed_loop = false;
}
else
{
residual.closed_loop = true;
}
residual.closed_loop =
fabs(closed_double) >= numeric_limits<double>::epsilon();
Copy link
Member Author

Choose a reason for hiding this comment

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

@jehicken I think this should really be an option that is set for the flow control residual, then it can have a typed true/false value and be more expressive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, I'm not sure why it is a data member. Would this be easy for you to fix? I only ask because I won't have time until summer to implement the change. Or, we could ignore it for now.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/8)

@@ -1,10 +1,12 @@
#ifndef MACH_MFEM_COMMON_INTEG
#define MACH_MFEM_COMMON_INTEG

#include <linalg/vector.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ clang-diagnostic-error ⚠️
linalg/vector.hpp file not found

@@ -83,7 +83,7 @@ TEST_CASE("MachInputs Scalar Input Test",
MachLinearForm lf(fes, fields);
lf.addDomainIntegrator(new TestMachLoadIntegrator);

MachLoad ml(lf);
MachLoad ml(std::move(lf));
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ clang-diagnostic-error ⚠️
call to implicitly-deleted copy constructor of mach::MachLinearForm

Copy link
Contributor

Choose a reason for hiding this comment

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

@tuckerbabcock Should we be concerned about this?

/// \param[in] pf_trans - the transformation between reference and physical
/// space
/// \param[out] peak_flux_bar - dJdB for the element
void AssembleRHSElementVect(const mfem::FiniteElement &pf_el,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-inconsistent-declaration-parameter-name ⚠️
function mach::ACLossFunctionalIntegratorPeakFluxSens::AssembleRHSElementVect has a definition with different parameter names

Copy link
Contributor

Choose a reason for hiding this comment

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

{
output_scalar_sens.at("frequency")
.AddDomainIntegrator(
new SteinmetzLossIntegratorFreqSens(primal_integ), *attr_marker);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ clang-diagnostic-error ⚠️
too many arguments to function call, expected single argument nlfi, have 2 arguments

output_scalar_sens.at("max_flux_magnitude")
.AddDomainIntegrator(
new SteinmetzLossIntegratorMaxFluxSens(primal_integ),
*attr_marker);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ clang-diagnostic-error ⚠️
too many arguments to function call, expected single argument nlfi, have 2 arguments

@@ -803,15 +805,15 @@
BoundaryEntropy(adept::Stack &diff_stack,
const mfem::FiniteElementCollection *fe_coll,
BCScaleFun scale,
const mfem::Vector &xc,
mfem::Vector xc,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the parameter xc is copied for each invocation but only used as a const reference; consider making it a const reference

Suggested change
mfem::Vector xc,
const mfem::Vector xc,

@@ -803,15 +805,15 @@
BoundaryEntropy(adept::Stack &diff_stack,
const mfem::FiniteElementCollection *fe_coll,
BCScaleFun scale,
const mfem::Vector &xc,
mfem::Vector xc,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the parameter xc is copied for each invocation but only used as a const reference; consider making it a const reference

Suggested change
mfem::Vector xc,
mfem::Vector& xc,

double len = 1.0)
: InviscidBoundaryIntegrator<BoundaryEntropy<dim, entvar>>(diff_stack,
fe_coll,
dim + 2,
1.0),
len_scale(len),
x_actuator(xc),
control_scale(scale)
x_actuator(std::move(xc)),
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-move-const-arg ⚠️
passing result of std::move() as a const reference argument; no move will actually happen

Suggested change
x_actuator(std::move(xc)),
x_actuator(xc)),

double len = 1.0)
: InviscidBoundaryIntegrator<BoundaryEntropy<dim, entvar>>(diff_stack,
fe_coll,
dim + 2,
1.0),
len_scale(len),
x_actuator(xc),
control_scale(scale)
x_actuator(std::move(xc)),
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-move-const-arg ⚠️
passing result of std::move() as a const reference argument; no move will actually happen

Suggested change
x_actuator(std::move(xc)),
x_actuator(std::move(xc),


mach::RelaxedNewton newton(MPI_COMM_SELF, newton_opts);
// mfem::NewtonSolver newton(MPI_COMM_SELF);
newton.SetPrintLevel(mfem::IterativeSolver::PrintLevel().All());
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ clang-diagnostic-error ⚠️
no member named PrintLevel in mfem::IterativeSolver

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (2/8)

@@ -1,6 +1,7 @@
#include <string>
#include <unordered_map>

#include "mach_load.hpp"
#include "mfem.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable state is not initialized

Suggested change
#include "mfem.hpp"
#include "math.h"
#include "mfem.hpp"

const std::string &wrt)
{
const MachInputs &inputs = *output.inputs;
double state = calcOutput(output.state_integ, inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable state is not initialized

Suggested change
double state = calcOutput(output.state_integ, inputs);
double state = NAN = calcOutput(output.state_integ, inputs);

{
const MachInputs &inputs = *output.inputs;
double state = calcOutput(output.state_integ, inputs);
double volume = calcOutput(output.volume, inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable volume is not initialized

Suggested change
double volume = calcOutput(output.volume, inputs);
double volume = NAN = calcOutput(output.volume, inputs);

mfem::Vector &wrt_bar)
{
const MachInputs &inputs = *output.inputs;
double state = calcOutput(output.state_integ, inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable state is not initialized

Suggested change
double state = calcOutput(output.state_integ, inputs);
double state = NAN = calcOutput(output.state_integ, inputs);

{
const MachInputs &inputs = *output.inputs;
double state = calcOutput(output.state_integ, inputs);
double volume = calcOutput(output.volume, inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable volume is not initialized

Suggested change
double volume = calcOutput(output.volume, inputs);
double volume = NAN = calcOutput(output.volume, inputs);

output.wire_length / (strand_area * output.strands_in_hand) * rho_dot;

double loss = pow(output.rms_current, 2) * R * sqrt(2);
double loss_dot = pow(output.rms_current, 2) * sqrt(2) * R_dot;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable loss_dot is not initialized

Suggested change
double loss_dot = pow(output.rms_current, 2) * sqrt(2) * R_dot;
double loss_dot = NAN = pow(output.rms_current, 2) * sqrt(2) * R_dot;

/// double loss = pow(output.rms_current, 2) * R * sqrt(2);
// double rms_current_bar = loss_bar * 2 * output.rms_current * R *
// sqrt(2);
double R_bar = loss_bar * pow(output.rms_current, 2) * sqrt(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable R_bar is not initialized

Suggested change
double R_bar = loss_bar * pow(output.rms_current, 2) * sqrt(2);
double R_bar = NAN = loss_bar * pow(output.rms_current, 2) * sqrt(2);

// volume does not depend on any of the inputs except mesh coords

/// double loss = pow(output.rms_current, 2) * R * sqrt(2);
double rms_current_bar = loss_bar * 2 * output.rms_current * R * sqrt(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable rms_current_bar is not initialized

Suggested change
double rms_current_bar = loss_bar * 2 * output.rms_current * R * sqrt(2);
double rms_current_bar = NAN = loss_bar * 2 * output.rms_current * R * sqrt(2);

/// double loss = pow(output.rms_current, 2) * R * sqrt(2);
// double rms_current_bar = loss_bar * 2 * output.rms_current * R *
// sqrt(2);
double R_bar = loss_bar * pow(output.rms_current, 2) * sqrt(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable R_bar is not initialized

Suggested change
double R_bar = loss_bar * pow(output.rms_current, 2) * sqrt(2);
double R_bar = NAN = loss_bar * pow(output.rms_current, 2) * sqrt(2);

/// double loss = pow(output.rms_current, 2) * R * sqrt(2);
// double rms_current_bar = loss_bar * 2 * output.rms_current * R *
// sqrt(2);
double R_bar = loss_bar * pow(output.rms_current, 2) * sqrt(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable R_bar is not initialized

Suggested change
double R_bar = loss_bar * pow(output.rms_current, 2) * sqrt(2);
double R_bar = NAN = loss_bar * pow(output.rms_current, 2) * sqrt(2);

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (3/8)

double R =
output.wire_length * rho / (strand_area * output.strands_in_hand);

double loss = pow(output.rms_current, 2) * R * sqrt(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable loss is not initialized

Suggested change
double loss = pow(output.rms_current, 2) * R * sqrt(2);
double loss = NAN = pow(output.rms_current, 2) * R * sqrt(2);

/// double loss = pow(output.rms_current, 2) * R * sqrt(2);
// double rms_current_bar =
// loss_bar * 2 * output.rms_current * R * sqrt(2);
double R_bar = loss_bar * pow(output.rms_current, 2) * sqrt(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable R_bar is not initialized

Suggested change
double R_bar = loss_bar * pow(output.rms_current, 2) * sqrt(2);
double R_bar = NAN = loss_bar * pow(output.rms_current, 2) * sqrt(2);

@@ -130,14 +130,14 @@ void InexactNewton::Mult(const Vector &b, Vector &x) const

if (norm <= norm_goal)
{
converged = 1;
converged = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-implicit-bool-conversion ⚠️
implicit conversion bool -> int

Suggested change
converged = true;
converged = 1;

final_iter = it;
break;
}

if (it >= max_iter)
{
converged = 0;
converged = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-implicit-bool-conversion ⚠️
implicit conversion bool -> int

Suggested change
converged = false;
converged = 0;

@@ -152,7 +152,7 @@

if (c_scale == 0.0)
{
converged = 0;
converged = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-implicit-bool-conversion ⚠️
implicit conversion bool -> int

Suggested change
converged = true;
converged = 1;

op.vectorJacobianProduct(out_bar, "mesh_coords", mesh_coords_bar);

auto dout_dmesh_v_local = mesh_pert * mesh_coords_bar;
double dout_dmesh_v;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dout_dmesh_v is not initialized

Suggested change
double dout_dmesh_v;
double dout_dmesh_v = NAN;

mfem::H1_FECollection fec(p, dim);
mfem::ParFiniteElementSpace fes(&mesh, &fec);

std::map<std::string, mach::FiniteElementState> fields;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable fields is not initialized

Suggested change
std::map<std::string, mach::FiniteElementState> fields;
std::map<std::string, mach::FiniteElementState> fields = 0;

#include <random>

#include "catch.hpp"
#include "mfem.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable pert is not initialized

Suggested change
#include "mfem.hpp"
#include "math.h"
#include "mfem.hpp"

{"current_density:box2", -current_density}
};

double pert = uniform_rand(gen);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable pert is not initialized

Suggested change
double pert = uniform_rand(gen);
double pert = NAN = uniform_rand(gen);

jacobianVectorProduct(res, pert_vec, "current_density:box1", res_dot);
double drdp_fwd = res_bar * res_dot;

double drdp_rev = vectorJacobianProduct(res, res_bar, "current_density:box1") * pert;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable drdp_rev is not initialized

Suggested change
double drdp_rev = vectorJacobianProduct(res, res_bar, "current_density:box1") * pert;
double drdp_rev = NAN = vectorJacobianProduct(res, res_bar, "current_density:box1") * pert;

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (4/8)

mfem::H1_FECollection fec(p, dim);
mfem::ParFiniteElementSpace fes(&mesh, &fec);

std::map<std::string, mach::FiniteElementState> fields;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable fields is not initialized

Suggested change
std::map<std::string, mach::FiniteElementState> fields;
std::map<std::string, mach::FiniteElementState> fields = 0;

mfem::H1_FECollection fec(p, dim);
mfem::ParFiniteElementSpace fes(&mesh, &fec);

std::map<std::string, mach::FiniteElementState> fields;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable fields is not initialized

Suggested change
std::map<std::string, mach::FiniteElementState> fields;
std::map<std::string, mach::FiniteElementState> fields = 0;

#include "finite_element_state.hpp"
#include "functional_output.hpp"
#include "magnetostatic_load.hpp"
#include "mfem.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fwd is not initialized

Suggested change
#include "mfem.hpp"
#include "math.h"
#include "mfem.hpp"

mfem::Vector adjoint_vec(&adjoint, 1);

setInputs(fun, inputs);
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "mesh_coords");
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fwd is not initialized

Suggested change
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "mesh_coords");
double dfdp_fwd = NAN = adjoint * jacobianVectorProduct(fun, pert_vec, "mesh_coords");


// now compute the finite-difference approximation...
mesh_coords_tv.Add(delta, pert_vec);
double dfdp_fd_p = calcOutput(fun, inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fd_p is not initialized

Suggested change
double dfdp_fd_p = calcOutput(fun, inputs);
double dfdp_fd_p = NAN = calcOutput(fun, inputs);

double dfdp_fd_p = calcOutput(fun, inputs);

inputs["stack_length"] = stack_length - pert * delta;
double dfdp_fd_m = calcOutput(fun, inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fd_m is not initialized

Suggested change
double dfdp_fd_m = calcOutput(fun, inputs);
double dfdp_fd_m = NAN = calcOutput(fun, inputs);

mfem::H1_FECollection fec(p, dim);
mfem::ParFiniteElementSpace fes(&mesh, &fec);

std::map<std::string, mach::FiniteElementState> fields;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable fields is not initialized

Suggested change
std::map<std::string, mach::FiniteElementState> fields;
std::map<std::string, mach::FiniteElementState> fields = 0;

mfem::Vector adjoint_vec(&adjoint, 1);

setInputs(fun, inputs);
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "strands_in_hand");
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fwd is not initialized

Suggested change
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "strands_in_hand");
double dfdp_fwd = NAN = adjoint * jacobianVectorProduct(fun, pert_vec, "strands_in_hand");


setInputs(fun, inputs);
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "strands_in_hand");
double dfdp_rev = vectorJacobianProduct(fun, adjoint_vec, "strands_in_hand") * pert;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_rev is not initialized

Suggested change
double dfdp_rev = vectorJacobianProduct(fun, adjoint_vec, "strands_in_hand") * pert;
double dfdp_rev = NAN = vectorJacobianProduct(fun, adjoint_vec, "strands_in_hand") * pert;


// now compute the finite-difference approximation...
inputs["strands_in_hand"] = strands_in_hand + pert * delta;
double dfdp_fd_p = calcOutput(fun, inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fd_p is not initialized

Suggested change
double dfdp_fd_p = calcOutput(fun, inputs);
double dfdp_fd_p = NAN = calcOutput(fun, inputs);

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (5/8)

double dfdp_fd_p = calcOutput(fun, inputs);

inputs["strands_in_hand"] = strands_in_hand - pert * delta;
double dfdp_fd_m = calcOutput(fun, inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fd_m is not initialized

Suggested change
double dfdp_fd_m = calcOutput(fun, inputs);
double dfdp_fd_m = NAN = calcOutput(fun, inputs);

mfem::H1_FECollection fec(p, dim);
mfem::ParFiniteElementSpace fes(&mesh, &fec);

std::map<std::string, mach::FiniteElementState> fields;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable fields is not initialized

Suggested change
std::map<std::string, mach::FiniteElementState> fields;
std::map<std::string, mach::FiniteElementState> fields = 0;

mfem::Vector adjoint_vec(&adjoint, 1);

setInputs(fun, inputs);
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "num_turns");
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fwd is not initialized

Suggested change
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "num_turns");
double dfdp_fwd = NAN = adjoint * jacobianVectorProduct(fun, pert_vec, "num_turns");


setInputs(fun, inputs);
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "num_turns");
double dfdp_rev = vectorJacobianProduct(fun, adjoint_vec, "num_turns") * pert;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_rev is not initialized

Suggested change
double dfdp_rev = vectorJacobianProduct(fun, adjoint_vec, "num_turns") * pert;
double dfdp_rev = NAN = vectorJacobianProduct(fun, adjoint_vec, "num_turns") * pert;


// now compute the finite-difference approximation...
inputs["num_turns"] = num_turns + pert * delta;
double dfdp_fd_p = calcOutput(fun, inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fd_p is not initialized

Suggested change
double dfdp_fd_p = calcOutput(fun, inputs);
double dfdp_fd_p = NAN = calcOutput(fun, inputs);

mfem::Vector adjoint_vec(&adjoint, 1);

setInputs(fun, inputs);
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "frequency");
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fwd is not initialized

Suggested change
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "frequency");
double dfdp_fwd = NAN = adjoint * jacobianVectorProduct(fun, pert_vec, "frequency");


setInputs(fun, inputs);
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "frequency");
double dfdp_rev = vectorJacobianProduct(fun, adjoint_vec, "frequency") * pert;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_rev is not initialized

Suggested change
double dfdp_rev = vectorJacobianProduct(fun, adjoint_vec, "frequency") * pert;
double dfdp_rev = NAN = vectorJacobianProduct(fun, adjoint_vec, "frequency") * pert;


// now compute the finite-difference approximation...
inputs["frequency"] = frequency + pert * delta;
double dfdp_fd_p = calcOutput(fun, inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fd_p is not initialized

Suggested change
double dfdp_fd_p = calcOutput(fun, inputs);
double dfdp_fd_p = NAN = calcOutput(fun, inputs);

double dfdp_fd_p = calcOutput(fun, inputs);

inputs["frequency"] = frequency - pert * delta;
double dfdp_fd_m = calcOutput(fun, inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fd_m is not initialized

Suggested change
double dfdp_fd_m = calcOutput(fun, inputs);
double dfdp_fd_m = NAN = calcOutput(fun, inputs);

mfem::H1_FECollection fec(p, dim);
mfem::ParFiniteElementSpace fes(&mesh, &fec);

std::map<std::string, mach::FiniteElementState> fields;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable fields is not initialized

Suggested change
std::map<std::string, mach::FiniteElementState> fields;
std::map<std::string, mach::FiniteElementState> fields = 0;

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (6/8)

mfem::Vector adjoint_vec(&adjoint, 1);

setInputs(fun, inputs);
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "max_flux_magnitude");
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fwd is not initialized

Suggested change
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "max_flux_magnitude");
double dfdp_fwd = NAN = adjoint * jacobianVectorProduct(fun, pert_vec, "max_flux_magnitude");


setInputs(fun, inputs);
double dfdp_fwd = adjoint * jacobianVectorProduct(fun, pert_vec, "max_flux_magnitude");
double dfdp_rev = vectorJacobianProduct(fun, adjoint_vec, "max_flux_magnitude") * pert;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_rev is not initialized

Suggested change
double dfdp_rev = vectorJacobianProduct(fun, adjoint_vec, "max_flux_magnitude") * pert;
double dfdp_rev = NAN = vectorJacobianProduct(fun, adjoint_vec, "max_flux_magnitude") * pert;


// now compute the finite-difference approximation...
inputs["max_flux_magnitude"] = max_flux_magnitude + pert * delta;
double dfdp_fd_p = calcOutput(fun, inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fd_p is not initialized

Suggested change
double dfdp_fd_p = calcOutput(fun, inputs);
double dfdp_fd_p = NAN = calcOutput(fun, inputs);

double dfdp_fd_p = calcOutput(fun, inputs);

inputs["max_flux_magnitude"] = max_flux_magnitude - pert * delta;
double dfdp_fd_m = calcOutput(fun, inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable dfdp_fd_m is not initialized

Suggested change
double dfdp_fd_m = calcOutput(fun, inputs);
double dfdp_fd_m = NAN = calcOutput(fun, inputs);

mfem::H1_FECollection fec(p, dim);
mfem::ParFiniteElementSpace fes(&mesh, &fec);

std::map<std::string, mach::FiniteElementState> fields;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable fields is not initialized

Suggested change
std::map<std::string, mach::FiniteElementState> fields;
std::map<std::string, mach::FiniteElementState> fields = 0;

@@ -610,7 +612,7 @@
const mfem::FiniteElementCollection *fe_coll,
double Re_num,
double Pr_num,
const mfem::Vector &q_outflow,
mfem::Vector q_outflow,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the parameter q_outflow is copied for each invocation but only used as a const reference; consider making it a const reference

Suggested change
mfem::Vector q_outflow,
mfem::Vector& q_outflow,

@@ -620,7 +622,7 @@
Re(Re_num),
Pr(Pr_num),
mu(vis),
q_out(q_outflow),
q_out(std::move(q_outflow)),
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-move-const-arg ⚠️
passing result of std::move() as a const reference argument; no move will actually happen

Suggested change
q_out(std::move(q_outflow)),
q_out(q_outflow)),

@@ -620,7 +622,7 @@
Re(Re_num),
Pr(Pr_num),
mu(vis),
q_out(q_outflow),
q_out(std::move(q_outflow)),
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-move-const-arg ⚠️
passing result of std::move() as a const reference argument; no move will actually happen

Suggested change
q_out(std::move(q_outflow)),
q_out(std::move(q_outflow),

@@ -759,7 +761,7 @@
const mfem::FiniteElementCollection *fe_coll,
double Re_num,
double Pr_num,
const mfem::Vector &q_far,
mfem::Vector q_far,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the parameter q_far is copied for each invocation but only used as a const reference; consider making it a const reference

Suggested change
mfem::Vector q_far,
const mfem::Vector q_far,

@@ -759,7 +761,7 @@
const mfem::FiniteElementCollection *fe_coll,
double Re_num,
double Pr_num,
const mfem::Vector &q_far,
mfem::Vector q_far,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the parameter q_far is copied for each invocation but only used as a const reference; consider making it a const reference

Suggested change
mfem::Vector q_far,
mfem::Vector& q_far,

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (7/8)

@@ -769,7 +771,7 @@ class ViscousFarFieldBC
Re(Re_num),
Pr(Pr_num),
mu(vis),
qfs(q_far),
qfs(std::move(q_far)),
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-move-const-arg ⚠️
passing result of std::move() as a const reference argument; no move will actually happen

Suggested change
qfs(std::move(q_far)),
qfs(q_far)),

@@ -769,7 +771,7 @@
Re(Re_num),
Pr(Pr_num),
mu(vis),
qfs(q_far),
qfs(std::move(q_far)),
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-move-const-arg ⚠️
passing result of std::move() as a const reference argument; no move will actually happen

Suggested change
qfs(std::move(q_far)),
qfs(std::move(q_far),

@@ -1072,9 +1074,9 @@
const mfem::FiniteElementCollection *fe_coll,
double Re_num,
double Pr_num,
const mfem::Vector &q_ref,
mfem::Vector q_ref,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the parameter q_ref is copied for each invocation but only used as a const reference; consider making it a const reference

Suggested change
mfem::Vector q_ref,
const mfem::Vector q_ref,

@@ -1072,9 +1074,9 @@
const mfem::FiniteElementCollection *fe_coll,
double Re_num,
double Pr_num,
const mfem::Vector &q_ref,
mfem::Vector q_ref,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the parameter q_ref is copied for each invocation but only used as a const reference; consider making it a const reference

Suggested change
mfem::Vector q_ref,
mfem::Vector& q_ref,

BCScaleFun scale,
const mfem::Vector &xc,
mfem::Vector xc,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the parameter xc is copied for each invocation but only used as a const reference; consider making it a const reference

Suggested change
mfem::Vector xc,
const mfem::Vector xc,

auto space_dim = mesh->SpaceDimension();
// auto space_dim = mesh->Dimension();
if (space_dim == 3)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ bugprone-branch-clone ⚠️
repeated branch in conditional chain

auto &mesh_fes = fields.at("mesh_coords").space();
output_sens.emplace("mesh_coords", &mesh_fes);

if (attr_marker == nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ bugprone-branch-clone ⚠️
if with identical then and else branches

auto &mesh_fes = fields.at("mesh_coords").space();
output_sens.emplace("mesh_coords", &mesh_fes);

if (attr_marker == nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ bugprone-branch-clone ⚠️
if with identical then and else branches

output_sens.emplace("mesh_coords", &mesh_fes);

const auto &state_name = primal_integ.state_name();
if (attr_marker == nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ bugprone-branch-clone ⚠️
if with identical then and else branches

output_sens.emplace("mesh_coords", &mesh_fes);

const auto &state_name = primal_integ.state_name();
if (attr_marker == nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ bugprone-branch-clone ⚠️
if with identical then and else branches

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (8/8)

auto &mesh_fes = fields.at("mesh_coords").space();
output_sens.emplace("mesh_coords", &mesh_fes);

if (attr_marker == nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ bugprone-branch-clone ⚠️
if with identical then and else branches

auto &mesh_fes = fields.at("mesh_coords").space();
output_sens.emplace("mesh_coords", &mesh_fes);

if (attr_marker == nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ bugprone-branch-clone ⚠️
if with identical then and else branches

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

jac.Mult(v, jac_v);

// now compute the finite-difference approximation...
GridFunction r(&fes), jac_v_fd(&fes);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-isolate-declaration ⚠️
multiple declarations in a single statement reduces readability

Suggested change
GridFunction r(&fes), jac_v_fd(&fes);
GridFunction r(&fes);
GridFunction jac_v_fd(&fes);

const mfem::Vector &elfun,
mfem::Vector &elfun_bar) override;

const std::string &state_name() { return _state_name; }
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-make-member-function-const ⚠️
method state_name can be made const

Suggested change
const std::string &state_name() { return _state_name; }
const std::string &state_name() const { return _state_name; }

const mfem::Vector &elfun,
mfem::Vector &elfun_bar) override;

const std::string &state_name() { return _state_name; }
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-make-member-function-const ⚠️
method state_name can be made const

Suggested change
const std::string &state_name() { return _state_name; }
const std::string &state_name() const { return _state_name; }

#include "mfem_common_integ.hpp"

#include "magnetostatic_residual.hpp"

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable pert is not initialized

Suggested change
#include <math.h>

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

#include "mfem_common_integ.hpp"

#include "magnetostatic_residual.hpp"

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ cppcoreguidelines-init-variables ⚠️
variable pert is not initialized

Suggested change
#include <math.h>

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

const nlohmann::json &options)
: output(fields.at("peak_flux").space(), fields),
const nlohmann::json &options,
std::string attr_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the parameter attr_name is copied for each invocation but only used as a const reference; consider making it a const reference

Suggested change
std::string attr_name)
const std::string attr_name)

const nlohmann::json &options)
: output(fields.at("peak_flux").space(), fields),
const nlohmann::json &options,
std::string attr_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the parameter attr_name is copied for each invocation but only used as a const reference; consider making it a const reference

Suggested change
std::string attr_name)
std::string& attr_name)

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