[FastPR][Core] Geometries PrintData minor improvement #11446
Conversation
rubenzorrilla
added
Kratos Core
FastPR
Not sure at all. Note that the Jacobian might not be defined for all the geometries. |
| for (unsigned int i = 0; i < this->size(); ++i) { | ||
| rOStream << "\tPoint " << i + 1 << "\t : "; | ||
| mPoints[i].PrintData(rOStream); | ||
| if (mPoints(i) != nullptr) { |
There was a problem hiding this comment.
this seems like a big change
are we sure we want to allow null-points?
I think better not, imagine all the checks we would need to introduce 😅
There was a problem hiding this comment.
We are already allowing them. All the prototype geometries use nullptr points.
There was a problem hiding this comment.
I mean, I don't mind dropping this, the point is that I ended up spending more time debugging this than the stuff I wanted to 😄. IMO it makes sense to do this check for the output functions as it might be quite helpful for debugging.
There was a problem hiding this comment.
I see, I made another suggestion :)
There was a problem hiding this comment.
I know this was implemented a long time ago, but now we have C++17 so there's no point in risking segfaults because of naked raw pointers => I suggest wrapping them in an std::optional.
If you're worried about messing up the memory alignment, I can come up with a version of optional for pointers that has an identical alignment to pointers, so that's not a problem.
There was a problem hiding this comment.
I think that even though @matekelemen 's comment is a good point, it will be still good to inform developers of the null points in the PrintData. In other words, introducing the std::optional when retrieving the points doesn't exclude informing about this situation to the user.
|
Finally I created a protected Note that the base geometry |
kratos/geometries/geometry.h
Outdated
| } | ||
| } | ||
| return is_valid; | ||
| return std::none_of(mPoints.ptr_begin(), mPoints.ptr_end(), [](auto& pPoint){return pPoint == nullptr;}); |
There was a problem hiding this comment.
great!
thanks to @matekelemen I learned abt this, so much nicer 🎉
| * @return true All points are valid | ||
| * @return false At least one point has nullptr value | ||
| */ | ||
| bool AllPointsAreValid() const |
There was a problem hiding this comment.
BTW covering this with a test would be amazing 👀
There was a problem hiding this comment.
I tried, but I didn't come up with a simple solution since it is protected...
Co-authored-by: Philipp Bucher <philipp.bucher@tum.de>
rubenzorrilla
deleted the
core/geometries-print-data-minor-improvements
branch
📝 Description
This adds a minor improvement to the
PrintDatafunction of the base geometry as well as to the "standard" derived ones. Basically, now it is checked whether the points pointers are null or not to avoid segfaulting when calling the method. If the pointers are null we will be informing the user of the situation. Note that we cannot throw an error as this might be a valid situation (e.g. the geometry prototypes).