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.

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

[Core] Removing core warnings #12709

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Conversation

roigcarlo
Copy link
Member

@roigcarlo roigcarlo commented Sep 30, 2024

📝 Description
Gets rid of some annoying warnings in the core:

  • Unused variables here and there
  • Deprecations related to DivideInputToPartitions and const versions of functions in IO (more than 2 years being marked as deprecated)

Comment on lines +90 to +94
/// Components const Constructor
GeometryContainer(
GeometriesMapType const& NewGeometries)
: mGeometries(NewGeometries)
{}
Copy link
Member Author

Choose a reason for hiding this comment

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

Please someone else from @KratosMultiphysics/technical-committee confirm this is fine.

It is needed to call the const version of the WriteGeometries from the model_part_io.h

rModelPart.Nodes().Sort(); // Avoid inadvertently reordering partway through

// Charlie: A write function should not modify the model part. This is a bug. If its broken needs to be fixed elsewhere.
// rModelPart.Nodes().Sort(); // Avoid inadvertently reordering partway through
Copy link
Member Author

@roigcarlo roigcarlo Oct 2, 2024

Choose a reason for hiding this comment

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

@sunethwarna I think this fix is yours, we need to find another solution or call the sort outside this function.

Copy link
Member

Choose a reason for hiding this comment

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

from my point of view this one is a blocker. This PR cannot go in until the PointerVectorSet is fully fixed

having said this i could accept a check here for the model_part.Nodes to be sorted. If it fails that is an error...

Comment on lines +3813 to +3814
// This needs to be somewhere else.
// r_nodes_array.Unique();
Copy link
Member Author

Choose a reason for hiding this comment

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

Someone from the meshing can tall me why we need to reorder and remove duplicates from the modelpart here? A workaround would be to create a print modelpart but we cannot modify the original in a write function

Copy link
Member

Choose a reason for hiding this comment

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

The problem is the operation or where the operation is placed?

Copy link
Member Author

@roigcarlo roigcarlo Oct 2, 2024

Choose a reason for hiding this comment

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

Where it is placed.

Right now is in a write method, which in the non deprecated IO needs a const & ModelPart, so we cannot do that. If the reorder is needed needs to be placed outside (which makes sense) but not sure where.

Copy link
Member

Choose a reason for hiding this comment

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

The code you modified I don't think it will work. You cannot have const model part in the function that literally is retrieving the new mesh

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep that's my point, that function should not be in charge of doing that. Ideally you should have:

void foo() {
    ModelPart my_model = GenerateModel()
    Write(my_model)
}

but for some reason, in the meshing, the `GenerateModel()` part is done inside the write. I am trying to understand where this write code is being called to split it in two parts (the part that modifies the modelpart and the part that actually writes it)

Copy link
Member

Choose a reason for hiding this comment

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

but this one is needed for correctness, so i can go in only when all the ongoing changes of the PointerVectorSet (including the "authority" stuff are merged)

Copy link
Contributor

Choose a reason for hiding this comment

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

including the "authority" stuff are merged

that's on me and will likely take ages

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.

sorry but you cannot simply take out the sort. This is the end of the road not the beginning...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants