search

FluidityProject / fluidity

Fluidity
http://fluidity-project.org
Other
366 stars 115 forks source link

Fix wrong Jacobian dimensions in transform_cvsurf_to_physical. #245

Closed stephankramer closed 4 years ago

stephankramer commented 4 years ago

The second dimension of J should be the surface dimension (i.e. dim-1), otherwise the matmul in line 2326 does not match. This is copying a smaller array into a larger, so should be safe - and I can't actually get my compiler to complain (even with debugging) - but this should hopefully fix #206.

angus-g commented 4 years ago

Looks like there's another related failure in transform_facet_to_physical_detwei in the calculation

     real, dimension(X%dim,X%dim-1) :: J

...

       J=matmul(X_f(:,:), x_shape_f%dn(:, gi, :))
Shape mismatch: The extent of dimension 2 of array J is 2 and the corresponding extent
of array <RHS expression> is 1
stephankramer commented 4 years ago

Can you give me a backtrace and the case that you ran this on?

angus-g commented 4 years ago

Happens on at least spherical_mappings_3d and gauss_point_buoyancy:

spherical_mappings_3d: forrtl: severe (408): fort: (33): Shape mismatch: The extent of dimension 2 of array J is 2 and the corresponding extent of array <RH
S expression> is 1
spherical_mappings_3d: 
spherical_mappings_3d: Image              PC                Routine            Line        Source             
spherical_mappings_3d: libHYPRE-2.18.2.s  000014A1A8D8BF89  for_emit_diagnost     Unknown  Unknown
spherical_mappings_3d: fluidity           0000000000EC9918  transform_element        1731  Transform_elements.F90
spherical_mappings_3d: fluidity           000000000128B1EB  populate_state_mo        2931  Populate_State.F90
spherical_mappings_3d: fluidity           0000000001252EF5  populate_state_mo         468  Populate_State.F90
spherical_mappings_3d: fluidity           000000000124ECC9  populate_state_mo         174  Populate_State.F90
spherical_mappings_3d: fluidity           000000000058C4BB  fluids_module_mp_         209  Fluids.F90
spherical_mappings_3d: fluidity           0000000000584AAF  mainfl                     67  mainfl.F90
spherical_mappings_3d: fluidity           000000000057B7A4  Unknown               Unknown  Unknown
spherical_mappings_3d: libc-2.28.so       000014A180AA2813  __libc_start_main     Unknown  Unknown
spherical_mappings_3d: fluidity           000000000057B34E  Unknown               Unknown  Unknown

I'll note that I used the following to get around it, but I don't know if it's strictly correct:

diff --git a/femtools/Transform_elements.F90 b/femtools/Transform_elements.F90
index d12e49c7e..9e4115639 100644
--- a/femtools/Transform_elements.F90
+++ b/femtools/Transform_elements.F90
@@ -1664,7 +1664,7 @@ contains

     ! Jacobian matrix and its inverse.
-    real, dimension(X%dim,X%dim-1) :: J
+    real, dimension(X%dim,mesh_dim(X)-1) :: J
     ! Determinant of J
     real :: detJ
     ! Whether the cache can be used
stephankramer commented 4 years ago

I think you are right. I've added your suggested fix to this branch

stephankramer commented 4 years ago

I've restarted the pr-merge: https://jenkins.ese.ic.ac.uk:1080/blue/organizations/jenkins/fluidity/detail/PR-245/5/pipeline I think you need to login to Jenkins to be able to restart. Don't know if you have a login, otherwise Tim will of course be happy to provide you with one. Will merge when this passes.