[CORE] Fixed issue with projection onto 2D2 line when input does not lie exactly on the line.

[rfaasse]

📝 Description
This PR aims to fix an issue found when testing the 2D2 line. When converting a point not exactly on the 2D2 Line, the projection was incorrect. By using an inner product instead of the norm2 distance to the input point to calculate the projected position on the line, we get expected results for the cases found in the added unit tests.

🆕 Changelog

• Fixed projection issue by using an inner product instead of using the distance from the points of the line to the input point
• Added unit tests

[avdg81]

Clean and clear improvement, with clear unit tests. I only have one very minor remark. Well done!

[loumalouomega]

Tests are failing. What is exactly the issue?

[loumalouomega]

Blocking as this requires further review.

[rfaasse]

Tests are failing. What is exactly the issue?

@loumalouomega This is a draft PR, we are currently looking into the issue and will request a review once the issue is solved.

[loumalouomega]

Tests are failing. What is exactly the issue?

@loumalouomega This is a draft PR, we are currently looking into the issue and will request a review once the issue is solved.

Tests passed now

[rfaasse]

Tests are failing. What is exactly the issue?

@loumalouomega This is a draft PR, we are currently looking into the issue and will request a review once the issue is solved.

Tests passed now

Indeed, we fixed the issue, it had to do with a tolerance which we believe should be removed at some point, but right now some functionality depends on this tolerance, meaning it is out of scope to do that now.

[loumalouomega]

I will approve as soon as @KratosMultiphysics/technical-committee agrees

[matekelemen]

No need to compute Length() twice.

Besides, the job of that tolerance is probably to prevent division by zero errors but there's a naked division on line 1071. I'd either get rid of tolerance completely (preferred - degenerate lines should be caught at construction) or apply it there as well to remain consistent.

[loumalouomega]

You are right about avoiding computing Length twice, but the tolerance is not because is degenerated, it is because the tolerance on the checks to avoid false negatives.

[matekelemen]

False negatives?

As I understand this function doesn't do point membership tests but transforms a point from global to local space.

[loumalouomega]

Yes, but that is used in the IsInside function, maybe the proper way should to check tolerance there (where Toelrance is an input)

[matekelemen]

Yup, I like that idea better.

[matekelemen]

By the way IsInside should probably be changed after this PR because an infinitely large cylinder would project to this line.

[rfaasse]

I agree, at first I removed the tolerance completely, but it failed some test cases in different applications, so it would require quite some effort to fix (simply adding the tolerance when calling IsInside didn't work unfortunately).

I can have a look again after I fixed the issue where using auto for an array1d gave issues, maybe the tolerance can now be removed safely.

However, if that's not the case and we need to change quite a bit of code, I would suggest to do that in a separate PR and keep this one focused on just fixing the projection and keep the tolerance that was already there.

[rfaasse]

Dear @loumalouomega,
Thank you for your suggestions to improve this PR. I would appreciate it however, if you would not push direct changes to the branch I proposed to be merged into master. Of course, I will take all comments into consideration and make changes accordingly.
Thank you in advance!

[avdg81]

@loumalouomega: My two cents:

1. I would appreciate it if you can explain the added value of the comment // The length when the code is self-explaining at this point (the variable is named length and the function that initializes it is named Length).
2. When the changes for this PR were made, we have followed the advice given in Martin Fowler's book "Refactoring". In the book he suggests to replace local variables by function calls that return the targeted value. However, I noticed that you replaced the two calls to Length by introducing a local variable. This seems to contradict Martin's advice. For your reference, I will quote his motivation (at page 14) as well as his thoughts on performance concerns (at page 64). May I ask what your point of view is after considering his advice?

[p. 14]
The great benefit of removing local variables is that it makes it much easier to do extractions, since there is less local scope to deal with. Indeed, usually I'll take out local variables before I do any extractions.

[p.64]
A common concern with refactoring is the effect it has on the performance of a program. To make the software easier to understand, I often make changes that will cause the program to run slower. This is an important issue. I don't belong to the school of thought that ignores performance in favor of design purity or in hopes of faster hardware. Software has been rejected for being too slow, and faster machines merely move the goal posts. Refactoring can certainly make software go more slowly--but it also makes the software more amenable to performance tuning. The secret to fast software, in all but hard real-time contexts, is to write tunable software first and then tune it for sufficient speed.

3. I agree with @rfaasse that pushing changes to somebody else's branch without her/his prior consent may not be perceived as constructive as you had intended. Could we agree that if you see room for improvement, you just add a substantiated suggestion for the programmer to consider? Please rest assured that when the branch owner (after reviewing your suggestion) agrees it is an improvement, it will be incorporated by her/him.
4. It has been noted before that blocking a pull request (PR) to prevent accidental merge should generally be avoided. Blocking a PR can be problematic, since only the person who blocked it can unblock it by giving her/his approval. And since this is a draft PR, merging is not possible anyway. With these considerations in mind, could you agree that blocking of a PR is only used as a last resort? For instance, when a PR has already been approved by someone else, but when you feel it is not ready yet for merging.

Thank you for your understanding and your willingness to improve the code base.

[loumalouomega]

@loumalouomega: My two cents:

1. I would appreciate it if you can explain the added value of the comment `// The length` when the code is self-explaining at this point (the variable is named `length` and the function that initializes it is named `Length`).

Length is a very expensive operation (it computes sqrt, which is very expensive operation), that's why in the search implemnations the distance is not computed., the square of the distance is computed. This is a geometry, a extendently used class everywhere by everyone, performance is key.

2. When the changes for this PR were made, we have followed the advice given in [Martin Fowler's book "Refactoring"](https://martinfowler.com/books/refactoring.html). In the book he suggests to replace local variables by function calls that return the targeted value. However, I noticed that you replaced the two calls to `Length` by introducing a local variable. This seems to contradict Martin's advice. For your reference, I will quote his motivation (at page 14) as well as his thoughts on performance concerns (at page 64). May I ask what your point of view is after considering his advice?

I don't know who is this person, but sure is not working in HPC where performance is key, even is that implies hardcode and duplicate code in some situations.

[p. 14]
The great benefit of removing local variables is that it makes it much easier to do extractions, since there is less local scope to deal with. Indeed, usually I'll take out local variables before I do any extractions.
[p.64]
A common concern with refactoring is the effect it has on the performance of a program. To make the software easier to understand, I often make changes that will cause the program to run slower. This is an important issue. I don't belong to the school of thought that ignores performance in favor of design purity or in hopes of faster hardware. Software has been rejected for being too slow, and faster machines merely move the goal posts. Refactoring can certainly make software go more slowly--but it also makes the software more amenable to performance tuning. The secret to fast software, in all but hard real-time contexts, is to write tunable software first and then tune it for sufficient speed.

I think that what here is said give me the reason.

3. I agree with @rfaasse that pushing changes to somebody else's branch without her/his prior consent may not be perceived as constructive as you had intended. Could we agree that if you see room for improvement, you just add a substantiated suggestion for the programmer to consider? Please rest assured that when the branch owner (after reviewing your suggestion) agrees it is an improvement, it will be incorporated by her/him.

Sorry, I will avoid that in teh future, it was a very small change and I did it in order to be less tiresome with very small changes.

4. It has been noted [before](https://github.com/KratosMultiphysics/Kratos/pull/11528#issuecomment-1699411474) that blocking a pull request (PR) to prevent accidental merge should generally be avoided. Blocking a PR can be problematic, since only the person who blocked it can unblock it by giving her/his approval. And since this is a _draft_ PR, merging is not possible anyway. With these considerations in mind, could you agree that blocking of a PR is only used as a _last resort_? For instance, when a PR has already been approved by someone else, but when you feel it is not ready yet for merging.

I am not blocking to prevent accidental merge, here I am blockign until @KratosMultiphysics/technical-committee agrees as geometries are very important classes and can not be modified without their permision, even if that implies changes taking a lot of time (I have several PR that took 2 years to merge and one still waiting for approval because of this).

[loumalouomega]

@loumalouomega: My two cents:

1. I would appreciate it if you can explain the added value of the comment `// The length` when the code is self-explaining at this point (the variable is named `length` and the function that initializes it is named `Length`).

Length is a very expensive operation (it computes sqrt, which is very expensive operation), that's why in the search implemnations the distance is not computed., the square of the distance is computed. This is a geometry, a extendently used class everywhere by everyone, performance is key.

FYI: https://en.wikipedia.org/wiki/Fast_inverse_square_root

[rfaasse]

Hi @KratosMultiphysics/technical-committee, would it be possible to have another look at this PR? It fixes quite a fundamental issue with projections onto a 2D2 line geometry and includes unit tests for the situations which result in incorrect results with the current code.

[RiccardoRossi]

@KratosMultiphysics/technical-committee agrees in principle, nevertheless we would observe that the tolerance is not needed at all...which i guess goes on the line of what @loumalouomega was observing

[RiccardoRossi]

i understand that this tolerance was hardcoded before, but it is definitely now too nice ( for example it may fail for very small geometries).

Not telling that we should consider this as a blocker, but this is not a very robust solution.

Our point here is: why do we need the tolerance at all? anyhow you are assuming lenght to be different from zero, otherwise it would fail already in 1093

[rfaasse]

Yes, I completely agree that we shouldn't have the tolerance here at all. I tried before to remove it, but the IsInside function seems to expect the results from PointLocalCoordinates to be as they are (as @loumalouomega mentioned), so tests started failing.

To keep the scope of this PR limited, I only fixed the projection itself, but kept the conversion of the projection to a $\xi$ value as is.

As proposed by @loumalouomega and @matekelemen, the IsInside function should be changed (i.e. the tolerance should be applied there instead of in PointLocalCoordinates), so I would propose to investigate and change this in a separate small-scoped PR.