Add pybindings for Jacobian diagnostic #4685
Conversation
| TargetFrame>&, | ||
| const tnsr::I<DataVector, Dim, TargetFrame>&, const ::Mesh<Dim>&)>( | ||
| &::domain::jacobian_diagnostic<Dim, TargetFrame>), | ||
| py::arg("jacobian"), py::arg("mapped_coords"), py::arg("mesh")); |
There was a problem hiding this comment.
Name the second arg TargetFrame == Frame::Inertial ? "inertial_coords" : "grid_coords". Then the py script will get the inertial coords from the volume data file by default, without having to specify the dataset name.
There was a problem hiding this comment.
Cool! I didn't know these were automagically available.
There was a problem hiding this comment.
This doesn't compile @nilsvu ... I get this error:
/Users/geoffrey/Codes/spectre/spectre/src/Domain/Python/JacobianDiagnostic.cpp:31:15: error: 'TargetFrame' does not refer to a value
py::arg(TargetFrame == Frame::Inertial ? "inertial_coords"
^
/Users/geoffrey/Codes/spectre/spectre/src/Domain/Python/JacobianDiagnostic.cpp:21:32: note: declared here
template <size_t Dim, typename TargetFrame>
^
/Users/geoffrey/Codes/spectre/spectre/src/Domain/Python/JacobianDiagnostic.cpp:31:46: error: expected '(' for function-style cast or type construction
py::arg(TargetFrame == Frame::Inertial ? "inertial_coords"
Is there a way to do this with std::is_same? I tried but didn't get anything to build
There was a problem hiding this comment.
Right, frames are structs and no enums. std::is_same_v should work.
| jac_diag = jacobian_diagnostic(jac, mapped_coordinates, mesh) | ||
| expected_jac_diag = tnsr.I[DataVector, 1, | ||
| Frame.ElementLogical](num_points=4, fill=0.) | ||
| self.assertAlmostEqual(jac_diag[0], expected_jac_diag[0]) |
There was a problem hiding this comment.
Does this work? You're comparing arrays, so I'd do import numpy.testing as npt and npt.assert_allclose(jac_diag, expected).
There was a problem hiding this comment.
Works because it's in 1D, so only one component. I did not know about assert_allclose, but I'm happy to switch to that!
There was a problem hiding this comment.
Only one component but 4 points. Not sure what exactly happens without all_close.
|
@nilsvu is the test failure something I messed up? Here's the error: I don't see how I could have broken this or why only this one test failed, but maybe I'm missing something? |
|
@geoffrey4444 the failing build is the one with pybindings disabled. It fails the C++ test because it uses the same Python file for random-value tests as the Python test, which imports the Domain pybindings. Just keep the random-value test in a separate file, not in the Python/ subdirectory. |
geoffrey4444
force-pushed
the
pybind_jacdiag
branch
2 times, most recently
from
7c705aa
to
ec43e1e
Compare
| @@ -31,6 +31,12 @@ spectre_add_python_bindings_test( | |||
| "Unit;Domain" | |||
| PyDomain) | |||
|
|
|||
| spectre_add_python_bindings_test( | |||
| "Unit.Domain.Python.JacobianDiagnostic" | |||
| Test_JacobianDiagnosticPythonBinding.py | |||
There was a problem hiding this comment.
Please just name this Test_JacobianDiagnostic.py and move the other file up one directory level. Also, name the other file just JacobianDiagnostic.py. That's what we do for all other random-value tests.
|
I don't think the macOS CI failure is related to this PR...it complains about some packages like hdf5 not being installed |
geoffrey4444
force-pushed
the
pybind_jacdiag
branch
from
ec43e1e
to
9ae6539
Compare
Proposed changes
Add python bindings to the jacobian diagnostic.
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments