Improper bounds checking in v_PhysEvaluate among other problems
Hi all, I've been using Nektar++ to prepare some data for post-processing and in reading the code base, I feel that the conversions between collapsed and cartesian coordinates causes some confusion in the code. For instance, in StdExpansions3d::v_PhysEvaluate <http://doc.nektar.info/latest/class_nektar_1_1_std_regions_1_1_std_expansion3_d.html>, the coordinates are only checked to be in the standard hex yet all 3d standard regions use this function, meaning bounds are improperly check for evaluation in all of these cases. This looks like someone meant to check the collapsed coordinates (though there still could be a bug if this was the case). Similarly, various comments throughout the code base sound just a bit confused. For instance, in TetGeom::v_ContainsPoint <http://doc.nektar.info/latest/_tet_geom_8cpp_source.html#l00108>, there is a comment "// Convert to the local (eta) coordinates)" when clearly the conversion is to the local Cartesian coordinates. This bugs me because eta (in all papers and texts that I can find!) is used for Cartesian points. Or the function GetCoord <http://doc.nektar.info/latest/class_nektar_1_1_spatial_domains_1_1_geometry.html#a02b917ca1e3e5e863ad436afe0c011a0> says in its doc strings say it takes "local collapsed coordinate Lcoord", but if I follow it to the appropriate implementation <http://doc.nektar.info/latest/class_nektar_1_1_std_regions_1_1_std_expansion2_d.html#ad6e59b37194068a949d7cf26bf44655d> for say a quad, the argument is converted to collapsed coordinates! This is really confusing for someone trying to determine the mathematical meaning of the code - especially if you are using the suggested texts as references. Beyond fixing things noted here, this leads me to give a suggestion: Why not provide a different type for floats that are in collapsed coordinates vs. cartesian vs. global? This ought to be no cost, and it would make the code base easier to read/prevent these sorts of issues. Thanks for listening, Teo.
participants (1)
-
Teodoro Collin