[Core] Introducing proper insert method to PointerVectorSet #12087

sunethwarna · 2024-02-19T14:08:52Z

📝 Description
This PR is introducing the PointerVectorSet::insert method which is suppose to replace the currently used PointerVectorSet::push_back method. This is one of many PRs addressing the issue #11363 .

❗ This is one of many PRs which will address the points mentioned in #11363.

Followings chages are done:

The insert(pointer) method is changed not to sort anymore, and to put the new element in the correct position while keeping the sortedness in the PointerVectorSet. This assumes PointerVectorSet is already in a sorted state.
The insert(begin, end) method also changed not to sort the existing data, it sorts the incoming range and make it unique before inserting them to the mData. This also assumes PointerVectorSet is already in sorted state.
The insert(position_hint, pointer) uses the position hint if it is valid, otherwise it discards the position_hint and puts the new pointer in the correct place. This also assumes PointerVectorSet is already in sorted state.
The default TGetKeyType is changed to return memory locations if the provided TDataType does not have operator< and operator== defined. Otherwise, TGetKeyType returns the exact input object.

🆕 Changelog

Modify insert methods to assume PointerVectorSet is always sorted when inserting new elements.
Introduced SetFunction to get the memory location if operator< and operator== not defined, otherwise it returns the input object..
Changed default identification method of PointerVectorSet to be based on memory or exact objects by using SetFunction
Added tests for introduced insert methods

kratos/containers/pointer_vector_set.h

ddiezrod · 2024-02-23T13:49:02Z

ping @KratosMultiphysics/altair so that everyone is aware as this is an important change,

pooyan-dadvand

Thanks @sunethwarna! I am not sure about the set function implementation and I think the insert implementation is not coherent. Also, I like more the implementation of the range inserts than the value insert itself. I think the range ones are more clear and have less ifs

pooyan-dadvand · 2024-02-26T12:11:05Z

kratos/containers/pointer_vector_set.h

-
-        *i = pData;
-        return i;
+        } else if (EqualKeyTo(KeyOf(*position_hint))(*ptr_begin())) {


I think here should be else if (position_hint == cbegin())

I can change it :). As far as i see, current implementation is the same, except, it checks the value of the begin rather than the iterator.

But the value check should come in the main if like other statements of upper if elses

makes sense and done :)

pooyan-dadvand · 2024-02-26T12:16:21Z

kratos/containers/pointer_vector_set.h

     */
    template <class InputIterator>
-    void insert(InputIterator First, InputIterator Last)
+    void insert(InputIterator first, InputIterator last)


Shall we have an additional argument (or a separate method) for insert_sorted_values?

It is little bit dangerous. Thats why I put insert(const PointVectorSet& rOther) overload to avoid doing sorting and making it unique. insert(end(), value) is having O(N) cost if we are inserting a sorted array which can be used to construct a quick sorted PointerVectorSet.

Shall we have an additional argument (or a separate method) for insert_sorted_values?

definitely not. The whole point of refactoring PointerVectorSet is to enforce its contract (sortedness and uniqueness) regardless of user input.

Agree with @sunethwarna that adding ordered set of entities is far faster than insert without assuming that. However creating the first PointerVectorSet would involve the sort.

Disagree with @matekelemen in the priority of robustness over performance for this case that the complexity of the algorithm changes from N to NlogN. Please note that in most of the mesh generation modelers we are adding millions of entities (to sub-modelparts) which we create with ordered indices (starting from the largest) and the first sort of those is an unnecessary operation

Nevertheless, I agree that this can be added afterward

pooyan-dadvand · 2024-02-26T12:27:32Z

kratos/containers/set_function.h

+     * @param data The input object of type TDataType.
+     * @return A non-const reference pointer or reference to the same object as provided in the parameter.
+     */
+    std::conditional_t<HasOperatorsDefined, TDataType&, TDataType*> operator()(TDataType& rData)


This is somehow weird to return different types and the final objective is not clear if one is not familiar with the implementation. I think it would be more clear to have the operator < and == defined here properly depending if the value has those defines or not

Then the question is, what is the key_type of a PointerVectorSet<SomeDataType> where the SomeDataType does not have the operator< and/or operator== defined?

We need to put somewhere this conditional_t if we want to illustrate that if they use PointerVectorSet with TDataType which does not have the operator< and operator== defined.

The above solution was the least intrusive AFAIK.

Now I see, This is the GetKey function which make sense to return the pointer instead of value if the value is not comparable. Then I would add the GetKey(or an equivalent) to the name to be more clear. (I admit that mine wasn't clear neither 😅)

I agree with the behavior then!

…_set/introduce_insert

sunethwarna · 2024-02-26T22:48:36Z

@pooyan-dadvand I addressed your suggestions/comments. Could you have a look?

…_set/introduce_insert

pooyan-dadvand

Already in a good shape! Some minor comments

pooyan-dadvand · 2024-02-27T16:28:28Z

kratos/containers/pointer_vector_set.h

+            {
+                // all 4 bounds are equal, hence this can be inserted without checking further
+                if (lower_bound_first == mData.end()) {
+                    for (auto it = first; it != last; ++it) {


Wouldn't be better to reserve first?

pooyan-dadvand · 2024-03-11T08:35:47Z

kratos/containers/pointer_vector_set.h

                return iterator(mData.end() - 1);
+            } else {
+                // given position is invalid. Hence, discarding the hint.
+                return insert(value);


Maybe a minor improvement, but I would not calling the insert here and just add the last statement of ifs in insert to avoid reevaluating for push_back

That's not possible, I need to add all if statements, because we need to do all the checks if the hint is discarded.

pooyan-dadvand · 2024-03-11T08:36:51Z

kratos/containers/pointer_vector_set.h

+                return mData.insert(mData.begin(), value);
+            } else {
+                // given position is invalid. Hence, discarding the hint.
+                return insert(value);


As explained before.

pooyan-dadvand · 2024-03-11T08:36:58Z

kratos/containers/pointer_vector_set.h

+                return mData.insert(mData.begin() + (position_hint - cbegin()), value);
+            } else {
+                // given position is invalid. Hence, discarding the hint.
+                return insert(value);


As explained before.

pooyan-dadvand · 2024-03-11T08:42:37Z

kratos/containers/set_function.h

+ * @author Suneth Warnakulasuriya
+ */
+template<class TDataType>
+class SetFunction


Would you please give a more descriptive name to this function? PointerSetGetKey?

done :)... Changed to KeyGenerator.

…_set/introduce_insert

sunethwarna · 2024-06-05T10:04:30Z

@pooyan-dadvand I addressed your comments. Could you take a look :)

…_set/introduce_insert

pooyan-dadvand

Thanks!

pooyan-dadvand · 2024-06-10T09:40:00Z

kratos/containers/pointer_vector_set.h

                return iterator(mData.end() - 1);
+            } else {
+                // given position is invalid. Hence, discarding the hint.
+                return insert(value);


sunethwarna added 4 commits February 19, 2024 12:58

implement set_address_identity_function

e1702fd

use set_address_identity_function

0520260

add insert methods to ptrvecset

abaeedd

add insert tests

4b34963

sunethwarna requested a review from a team as a code owner February 19, 2024 14:08

sunethwarna self-assigned this Feb 19, 2024

sunethwarna requested review from a team and removed request for a team February 19, 2024 14:09

sunethwarna added Behaviour Change changes behaviour but not the API Kratos Core C++ labels Feb 19, 2024

sunethwarna requested review from loumalouomega, roigcarlo, pooyan-dadvand, RiccardoRossi, rubenzorrilla, philbucher and matekelemen February 19, 2024 14:09

RiccardoRossi reviewed Feb 19, 2024

View reviewed changes

kratos/containers/pointer_vector_set.h Show resolved Hide resolved

kratos/containers/pointer_vector_set.h Show resolved Hide resolved

sunethwarna added 7 commits February 19, 2024 23:50

set mSortedPartSize = mData.size()

1a226e4

add the inserts to the end

770d666

revert elimination builder and solver changes

b3a9a76

change dofarraytype

b0ef86b

fix ranged insert bug with end

f5a7f05

fix mmg reordering

8dfbe09

fix dof updater

f2f3dcc

sunethwarna requested a review from RiccardoRossi February 20, 2024 13:35

sunethwarna added 2 commits February 20, 2024 15:58

add additional reserve

1c9af8e

add another insertion method

d0ef0e7

sunethwarna mentioned this pull request Feb 21, 2024

[Core] Replace core PointerVectorSet::push_back uses with relevant PointerVectorSet::insert #12103

Draft

2 tasks

sunethwarna added 4 commits February 23, 2024 00:41

introduce set function

9c00f71

define IndexedObject operators

8b04cec

use set function in PointerVectorSet

e86088b

add operator== to closest_point

5760f73

sunethwarna requested a review from a team as a code owner February 23, 2024 08:08

sunethwarna requested a review from RiccardoRossi February 23, 2024 08:10

pooyan-dadvand requested changes Feb 26, 2024

View reviewed changes

sunethwarna added 2 commits February 26, 2024 23:43

Merge remote-tracking branch 'origin/master' into core/pointer_vector…

bc46807

…_set/introduce_insert

use cbegin()

6e7a68f

sunethwarna requested a review from pooyan-dadvand February 26, 2024 22:52

sunethwarna added 3 commits February 27, 2024 13:02

add additional insert mechanism and test

f604617

Merge remote-tracking branch 'origin/master' into core/pointer_vector…

b1bc015

…_set/introduce_insert

remove use of *::base()

1437a25

pooyan-dadvand requested changes Mar 11, 2024

View reviewed changes

pooyan-dadvand mentioned this pull request May 13, 2024

[Core] Add new AddNodesFromOrderedContainer to model part #11503

Open

sunethwarna added 3 commits May 29, 2024 06:48

Merge remote-tracking branch 'origin/master' into core/pointer_vector…

8c58384

…_set/introduce_insert

add reserve

f84b135

avoid calling insert

b82885c

sunethwarna added 2 commits June 5, 2024 12:05

change name

0555c33

add additional iterator tests

4e5f741

sunethwarna requested a review from pooyan-dadvand June 5, 2024 10:09

sunethwarna added 3 commits June 5, 2024 12:38

remove operator def

2cfaa7b

Merge remote-tracking branch 'origin/master' into core/pointer_vector…

ebea638

…_set/introduce_insert

revert

359f87e

pooyan-dadvand approved these changes Jun 10, 2024

View reviewed changes

sunethwarna merged commit 384b3cd into master Jun 10, 2024
11 checks passed

sunethwarna deleted the core/pointer_vector_set/introduce_insert branch June 10, 2024 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Introducing proper insert method to PointerVectorSet #12087

[Core] Introducing proper insert method to PointerVectorSet #12087

sunethwarna commented Feb 19, 2024 •

edited

Loading

ddiezrod commented Feb 23, 2024 •

edited

Loading

pooyan-dadvand left a comment

pooyan-dadvand Feb 26, 2024

sunethwarna Feb 26, 2024 •

edited

Loading

pooyan-dadvand Feb 26, 2024

sunethwarna Feb 26, 2024 •

edited

Loading

pooyan-dadvand Feb 26, 2024

sunethwarna Feb 26, 2024

matekelemen Feb 26, 2024

pooyan-dadvand Feb 26, 2024

pooyan-dadvand Feb 26, 2024

sunethwarna Feb 26, 2024 •

edited

Loading

pooyan-dadvand Feb 26, 2024

sunethwarna commented Feb 26, 2024

pooyan-dadvand left a comment

pooyan-dadvand Feb 27, 2024

sunethwarna May 29, 2024

pooyan-dadvand Mar 11, 2024

sunethwarna May 29, 2024 •

edited

Loading

pooyan-dadvand Jun 10, 2024

pooyan-dadvand Mar 11, 2024

sunethwarna May 29, 2024 •

edited

Loading

pooyan-dadvand Mar 11, 2024

sunethwarna May 29, 2024 •

edited

Loading

pooyan-dadvand Mar 11, 2024

sunethwarna Jun 5, 2024

sunethwarna commented Jun 5, 2024

pooyan-dadvand left a comment

pooyan-dadvand Jun 10, 2024

[Core] Introducing proper insert method to PointerVectorSet #12087

[Core] Introducing proper insert method to PointerVectorSet #12087

Conversation

sunethwarna commented Feb 19, 2024 • edited Loading

ddiezrod commented Feb 23, 2024 • edited Loading

pooyan-dadvand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna commented Feb 26, 2024

pooyan-dadvand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna commented Jun 5, 2024

pooyan-dadvand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna commented Feb 19, 2024 •

edited

Loading

ddiezrod commented Feb 23, 2024 •

edited

Loading

sunethwarna Feb 26, 2024 •

edited

Loading

sunethwarna Feb 26, 2024 •

edited

Loading

sunethwarna Feb 26, 2024 •

edited

Loading

sunethwarna May 29, 2024 •

edited

Loading

sunethwarna May 29, 2024 •

edited

Loading

sunethwarna May 29, 2024 •

edited

Loading