Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Core] Making PointerVectorSet to be always ordered and unique #12077

Closed
wants to merge 15 commits into from
Closed

[Core] Making PointerVectorSet to be always ordered and unique #12077

wants to merge 15 commits into from

Conversation

sunethwarna
Copy link
Member

@sunethwarna sunethwarna commented Feb 16, 2024 •
edited
Loading

📝 Description
❗ CI IS FAILING BECAUSE, I WANT TO CONFIRM THE PROPOSED CHANGES ARE OK FIRST BEFORE PROCEEDING, AFTERWARDS I WILL FIX THE PROBLEMS ❗

Finally got time to work on the issue #11363. This PR intends to make PointerVectorSet to be always sorted and unique. Following changes are made:

  • immutable and mutable find do not sort. It assumes the container is sorted and returns the found items.
  • count method is removed [count should be always one for the unique PointerVectorSet].
  • push_back will be deprecated, and will use insert
  • Sort and Unique will be deprecated, and will use Conform
  • ...

⚠️ Followings also needs to be addressed in future:

  • mutuable and immutable PointerVectorSet::GetContainer can kill the sorted orderedness of the PointerVectorSet from outside.

⚠️ Unfortunately, we will not be able to have the PointerVectorSet always ordered and unique:

  • The SetId method of Node, Condition, Element or any IndexedObject allows mutating the ids of the entities in the PointerVectorSet, hence making it unordered and not unique. Therefore, we still need to have either Sort and Unique methods, or I would like to introduce Conform method which does both, and deprecate Sort and Unique.
  • The SetId is heavily used, hence we cannot disallow it.
  • So if someone uses SetId, then they should call Conform afterwards, otherwise, results may be affected.
  • The danger is, the current implementation, and the proposed implementation find will fail, if the PointerVectorSet is not sorted and unique.

🧪 This is ready for review. CI may be failing because, We first need to agree on the suggested changes in this PR before proceeding further. @KratosMultiphysics/technical-committee

🆕 Changelog

  • PointerVectorSet::find immutable and mutable versions does not sort any more.
  • Removed PointerVectorSet::count method

@sunethwarna sunethwarna changed the title [Core] Making PointerVectorSet to be always ordered [Core] Making PointerVectorSet to be always ordered and unique Feb 16, 2024
RiccardoRossi
RiccardoRossi reviewed Feb 17, 2024
View reviewed changes
Copy link
Member

@RiccardoRossi RiccardoRossi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some questions

kratos/containers/pointer_vector_set.h Show resolved Hide resolved
kratos/containers/pointer_vector_set.h Outdated Show resolved Hide resolved
kratos/containers/pointer_vector_set.h Outdated
Sort();
std::unique(mData.begin(), mData.end(), EqualKeyTo());
std::sort(mData.begin(), mData.end());
auto last = std::unique(mData.begin(), mData.end(), EqualKeyTo());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not using unique?

kratos/containers/pointer_vector_set.h Show resolved Hide resolved
kratos/containers/pointer_vector_set.h Outdated
@@ -543,75 +529,125 @@ class PointerVectorSet final
*/
void push_back(TPointerType x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought we wanted to remove this method

Copy link
Member Author

@sunethwarna sunethwarna Feb 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be deprecated pointing to the insert. In this PR, I will only replace push_back uses in the core only. In another PR, I will change the application uses of push_back to keep the changes in this PR as small as possible.

kratos/containers/pointer_vector_set.h
// the position to insert is at the end.
mData.push_back(value);
return iterator(mData.end() - 1);
} else if (EqualKeyTo(KeyOf(*value))(*itr_pos)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as explained before.

kratos/containers/pointer_vector_set.h

*i = pData;
return i;
} else if (EqualKeyTo(KeyOf(position_hint))(*ptr_begin())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as explained before.

kratos/containers/pointer_vector_set.h Show resolved Hide resolved
kratos/includes/indexed_object.h
@@ -96,6 +96,18 @@ class IndexedObject
return rThisObject.Id();
}

template<class TObjectType>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

templating this is a bit dangerous. don't we want it to work only for the same type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following the signature of the operator() to be consistent. I was not sure why operator() was templated. @pooyan-dadvand ?

kratos/processes/calculate_embedded_nodal_variable_from_skin_process.h
@@ -599,12 +599,16 @@ class CalculateEmbeddedNodalVariableFromSkinProcess : public Process

// Populate the modelpart with all the nodes in NodesMap
// Note that a temporary vector is created from the set to add all nodes at once
PointerVectorSet<Node> tmp;
PointerVectorSet<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the defaults do not work here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nodes constructor with key is not available (it throws an error), if someone does not specify the second template argument to the PointerVectorSet, then it uses SetIdentityFunction, which will need to create a copy of the node by using the key and node consturctor which throws an error. This is only problematic for Node type only.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the ctor being called in this case.

Kratos/kratos/includes/node.h

Lines 117 to 128 in 8fa8e65

explicit Node(IndexType NewId )
: BaseType()
, Flags()
, mNodalData(NewId)
, mDofs()
, mData()
, mInitialPosition()
, mNodeLock()
{
KRATOS_ERROR << "Calling the default constructor for the node ... illegal operation!!" << std::endl;
CreateSolutionStepData();
}

@sunethwarna sunethwarna self-assigned this Feb 18, 2024
@sunethwarna sunethwarna marked this pull request as ready for review February 18, 2024 10:15
@sunethwarna sunethwarna requested a review from a team as a code owner February 18, 2024 10:15
@sunethwarna sunethwarna requested a review from a team as a code owner February 19, 2024 07:51
@sunethwarna
Copy link
Member Author

I am closing this because, this PR will be seperated into few different PRs. Keeping the branch to get these changes propery to new PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roigcarlo roigcarlo Awaiting requested review from roigcarlo

@pooyan-dadvand pooyan-dadvand Awaiting requested review from pooyan-dadvand

@rubenzorrilla rubenzorrilla Awaiting requested review from rubenzorrilla

@RiccardoRossi RiccardoRossi Awaiting requested review from RiccardoRossi

@loumalouomega loumalouomega Awaiting requested review from loumalouomega

@matekelemen matekelemen Awaiting requested review from matekelemen

@philbucher philbucher Awaiting requested review from philbucher

Assignees

@sunethwarna sunethwarna

Labels
API Breaker Behaviour Change changes behaviour but not the API Bugfix C++ Kratos Core
Projects
Technical Commiittee
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sunethwarna @RiccardoRossi