Psi4 Fix #4647
Psi4 Fix #4647
Conversation
| @@ -96,9 +96,6 @@ void test_psi_4(const RealDataType& used_for_size_real, | |||
| auto inertial_coords = | |||
| make_with_random_values<tnsr::I<RealDataType, 3, Frame::Inertial>>( | |||
| nn_generator, nn_distribution, used_for_size_real); | |||
| for (size_t i = 0; i < 3; ++i) { | |||
| inertial_coords.get(i)[0] = 0.0; | |||
| } | |||
There was a problem hiding this comment.
I get that this test as it was written doesn't work anymore...but might it be possible for you to contrive an example that does give a zero denominator after the Gram-Schmidt orthonormalization? No big deal to me if not, but I would have thought you could work out what e.g. y_hat is for the 0th point before normalization, then modify it so that its magnitude is in fact zero?
There was a problem hiding this comment.
If this isn't something quick to do, then I wouldn't sweat it
| @@ -69,27 +71,62 @@ void psi_4(const gsl::not_null<Scalar<ComplexDataVector>*> psi_4_result, | |||
| inverse_spatial_metric, cov_deriv_extrinsic_curvature, r_hat, | |||
| inverse_projection_tensor, projection_tensor, projection_up_lo, 1.0); | |||
|
|
|||
| auto& x_coord = get<::Tags::TempI<0, 3, Frame>>(temp_buffer); | |||
| // Gram-Schmidt x_hat | |||
There was a problem hiding this comment.
Clarify comment: state something like Gram-Schmidt x_hat, i.e. make x_hat a unit vector orthogonal to r_hat
|
|
||
| Variables<tmpl::list<::Tags::TempI<0, 3, Frame, ComplexDataVector>, | ||
| ::Tags::TempI<1, 3, Frame, ComplexDataVector>>> | ||
| y_hat_buffer{get<0>(inertial_coords).size()}; | ||
|
|
||
| // Grad-Schmidt y_hat |
There was a problem hiding this comment.
Clarify comment: state something like Gram-Schmidt y_hat, i.e. make y_hat a unit vector orthogonal to r_hat and x_hat
| ::Tags::Tempii<1, 3, Frame>, ::Tags::TempIj<0, 3, Frame>, | ||
| ::Tags::TempII<0, 3, Frame>>> | ||
| Variables< | ||
| tmpl::list<::Tags::TempScalar<0>, ::Tags::TempScalar<1>, |
There was a problem hiding this comment.
Just so I understand, can you please explain to me why you need the additional temporaries?
There was a problem hiding this comment.
I think the additional TempScalar can be safely removed, however the additional TempI's are needed since TempI<0 = r_hat, TempI<1 = x_coord then changed to y_coord when not needed anymore, TempI<2 = x_hat and finally TempI<3 = y_hat_not_complex. I carry out the computation for y_hat in non-complex DataVector form for easier computation and then switch it to a ComplexDataVector later on.
AlexCarpenter46
force-pushed
the
psi4_fix
branch
from
5469717
to
bff4cc3
Compare
|
I'm not sure why a test was cancelled or timed out |
Proposed changes
This PR is to fix the visualization issues I've been seeing with Psi4. Made x_hat and y_hat orthonormal to r_hat at every point, was previously was not aware that this was required, I assumed x_hat and y_hat were the cartesian basis vectors.
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.