Skip to content

[General] Fixing sparse bugs #14247

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

Merged
roigcarlo merged 6 commits into from
Mar 5, 2026
Merged

[General] Fixing sparse bugs #14247

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
82cac39
Using explicit types
roigcarlo Feb 27, 2026
04b5737
Semicolon
roigcarlo Feb 27, 2026
7684188
Using Correct datatypes
roigcarlo Feb 27, 2026
2a7972c
Missing serializer
roigcarlo Feb 27, 2026
1c25685
Replace include by explicit forward declaration
roigcarlo Mar 5, 2026
d2b160b
Wrong copypaste
roigcarlo Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions applications/GeoMechanicsApplication/custom_constitutive/tension_cutoff.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "includes/kratos_export_api.h"
#include "includes/smart_pointers.h"
#include "includes/ublas_interface.h"
#include "includes/serializer.h"

@avdg81 avdg81 Mar 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @roigcarlo,

I have to admit that I don't see why it would be required to have this include here. Class Serializer is forward-declared and the only usages in this header file are references to this class (which is sufficient information for the compiler at this point). The implementation file (tension_cutoff.cpp) does have the include that you've added here now, which makes sense, since there we actually do something with the serializer objects.

The other thing that confuses me is that all other builds on GitHub pass with the status quo (i.e. without the additional include). So I'm wondering whether there is anything special about the build configuration that you're trying? Or perhaps there's something else that I overlook? If you have any ideas, please let me know. Thanks.

@roigcarlo roigcarlo Mar 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hi @avdg81, that drove me crazy a little bit as well. This is what I think is happening:

In the case of the CI is never a problem because it uses unitary builds which in practice is like having a PCH on windows, so the serializer.h include may come from the stack of headers included in the other sources (unless tension_cutoff.cpp is the first source bundled)

The second problem is more subtle. I am not 100% sure about if this is the case, but is the only think I came up with after searching for a while: I assume that the intention is for the friend class Serializer to also act as implicit forward declaration, but that declaration exists inside a class inside the namespace Kratos, so while it is true that Serializer is implicitly declared as Kratos::Serializer it is not as ::Serializer so it is not in the global lookup namespace. Seems that MSVC and some versions of gcc/clang simply do not enforce this rule and allow the declaration to be seen so that would explain why it does not normally fails. As a matter of fact, this specific header compiled well for the windows release, it only failed in GCC 14.

P.s.

Every name first declared in a namespace is a member of that namespace. If a friend declaration in a non-local class first declares a class, function, class template, or function template, the friend is a member of the innermost enclosing namespace

This is the rule that should fr followed by the compiler but is being ignored.

@avdg81 avdg81 Mar 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @roigcarlo,
Thanks a lot for investigating my question. Perhaps then the better way to solve it is to have a forward declaration in the Kratos namespace and use a friend declaration like this: friend Serializer; (or should it be friend Kratos::Serializer;?). My point is: if we can use a forward declaration (and I think we can), then I would prefer that over an extra include. May I ask you to try this for me? Thanks. If it works, I'm happy to make this change myself using a new PR, since we have similar constructs in other classes, too.

@roigcarlo roigcarlo Mar 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@avdg81 Np,

namespace Kratos
{

class Serializer;

namespace Geo
{
class PrincipalStresses;
class SigmaTau;
} // namespace Geo

This pretty much solves the problem as well. 👍


namespace Kratos
{
Expand Down
10 changes: 6 additions & 4 deletions applications/SystemIdentificationApplication/custom_utilities/mask_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,9 @@ std::vector<std::tuple<std::vector<IndexType>, typename ContainerExpression<TCon
{
KRATOS_TRY

std::vector<std::tuple<std::vector<IndexType>, typename ContainerExpression<TContainerType>::Pointer>> cluster_data;
using DataType = std::tuple<std::vector<IndexType>, typename ContainerExpression<TContainerType>::Pointer>;

std::vector<DataType> cluster_data;
if (rMasksList.size() == 0) {
return cluster_data;
}
Expand Down Expand Up @@ -364,7 +366,7 @@ std::vector<std::tuple<std::vector<IndexType>, typename ContainerExpression<TCon
std::sort(mask_indices.begin(), mask_indices.end());

/// check against existing mask indices
auto p_itr = std::find_if(cluster_data.begin(), cluster_data.end(), [&mask_indices](const auto& rData){ return std::get<0>(rData) == mask_indices; });
auto p_itr = std::find_if(cluster_data.begin(), cluster_data.end(), [&mask_indices](const DataType& rData){ return std::get<0>(rData) == mask_indices; });
if (p_itr == cluster_data.end()) {
auto p_expression = LiteralFlatExpression<int>::Create(number_of_entities, {});
IndexPartition<IndexType>(number_of_entities).for_each([&p_expression](const auto Index){
Expand All @@ -380,9 +382,9 @@ std::vector<std::tuple<std::vector<IndexType>, typename ContainerExpression<TCon
}

// now fill in the cluster masks
IndexPartition<IndexType>(number_of_entities).for_each([&cluster_data, &domain_mask_indices, &cluster_mask_exps](const auto Index) {
IndexPartition<IndexType>(number_of_entities).for_each([&cluster_data, &domain_mask_indices, &cluster_mask_exps](const IndexType Index) {
const auto& mask_indices = domain_mask_indices[Index];
auto p_itr = std::find_if(cluster_data.begin(), cluster_data.end(), [&mask_indices](const auto& rData){ return std::get<0>(rData) == mask_indices; });
auto p_itr = std::find_if(cluster_data.begin(), cluster_data.end(), [&mask_indices](const DataType& rData){ return std::get<0>(rData) == mask_indices; });
const auto cluster_index = std::distance(cluster_data.begin(), p_itr);
*(cluster_mask_exps[cluster_index]->begin() + Index) = 1;
});
Expand Down
Loading