|
From: | Konstantinos Poulios |
Subject: | Re: [Getfem-commits] merge request for touched_region_for_projected_fem |
Date: | Wed, 10 Mar 2021 17:08:08 +0100 |
Kostas, hi,Did you have a chance to review my latest commits?Thanks,AndriyOn Fri, 5 Mar 2021 at 09:27, Konstantinos Poulios <logari81@googlemail.com> wrote:Thanks. I see your point about "intersected". I think "projected_target_region" is a good name.If you do this change I can squash and merge your commits.Best regardsKostasOn Thu, Mar 4, 2021 at 2:06 PM Andriy Andreykiv <andriy.andreykiv@gmail.com> wrote:Hi Konstantinos,Replaced almost all the auto's. Regarding intersected_target_region, that could be, but for me the word intersected would be appropriate if it was interpolated_fem where the regions intersect.Physically it feels like touch, but I agree that we have touch() in context_dependencies which might be confusing.Here I feel a need for the concept of projection. I personally need a name that condenses the sentence "region that contains part of the target where the source is projected on". Can we sayprojected_target_region? We can also go with supported_target_region()?Let me know what you think,AndriyOn Thu, 4 Mar 2021 at 12:38, Konstantinos Poulios <logari81@googlemail.com> wrote:should we also briefly discuss the name of the function? In mathematical terms I would call itsupport_regionBut a more popularized name might be easier to understand in general. However, I do not like "touched" because "touch" is typically used in programming for denoting change in state, so your current name I understand it as a region that has state A and changes to state B. We could instead call itintersected_target_regionor something similar. Other ideas?In general I think we should spend some effort in good names because once a name is in the API it is more difficult to remove.Best regardsKostasOn Thu, Mar 4, 2021 at 12:31 PM Konstantinos Poulios <logari81@googlemail.com> wrote:Hi Andriy,Thanks, I see how it can be useful. Could I ask you to reduce the use of auto for this commit? For example it does not make much sense to use auto for bool. In general my preference for the GetFEM codebase is to use auto only if some type is particularly long and makes the code significantly less readable. Otherwise the type of the variables is useful information for people that will read and try to understand the code later.There is also a typo in a comment. It should be "Gauss".Best regardsKostasOn Thu, Mar 4, 2021 at 11:32 AM Andriy Andreykiv <andriy.andreykiv@gmail.com> wrote:Dear Yves and Konstantinus,Kind request to review and merge touched_region_for_projected_fem branch.It introduces a method for projected_fem that extracts a region from the target that is actually touched by the source.I use this region to integrate my mortar terms on.Best regards,Andriy
[Prev in Thread] | Current Thread | [Next in Thread] |