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

[core] Variable deregistry #12554

Merged
merged 4 commits into from
Nov 13, 2024
Merged

[core] Variable deregistry #12554

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57d5e4e
Add RegistryItem::IsSameType
jcotela Jul 17, 2024
ecccd9f
Register variable by application
jcotela Jul 17, 2024
0ab600f
Deregister variables when destroying application
jcotela Jul 17, 2024
b54b4d1
Merging master
jcotela Nov 13, 2024
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
36 changes: 17 additions & 19 deletions kratos/containers/variable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,7 @@ class Variable : public VariableData
: VariableData(NewName, sizeof(TDataType)),
mZero(Zero),
mpTimeDerivativeVariable(pTimeDerivativeVariable)
{
RegisterThisVariable();
}
{}
/**
* @brief Constructor with specific name and zero value
* @param NewName The name to be assigned to the new variable
Expand All @@ -110,9 +108,7 @@ class Variable : public VariableData
: VariableData(NewName, sizeof(TDataType)),
mZero(TDataType()),
mpTimeDerivativeVariable(pTimeDerivativeVariable)
{
RegisterThisVariable();
}
{}

/**
* @brief Constructor for creating a component of other variable
Expand All @@ -128,9 +124,7 @@ class Variable : public VariableData
)
: VariableData(rNewName, sizeof(TDataType), pSourceVariable, ComponentIndex),
mZero(Zero)
{
RegisterThisVariable();
}
{}

/**
* @brief Constructor for creating a component of other variable
Expand All @@ -149,9 +143,7 @@ class Variable : public VariableData
: VariableData(rNewName, sizeof(TDataType), pSourceVariable, ComponentIndex),
mZero(Zero),
mpTimeDerivativeVariable(pTimeDerivativeVariable)
{
RegisterThisVariable();
}
{}

/**
* Copy constructor.
Expand Down Expand Up @@ -325,6 +317,19 @@ class Variable : public VariableData
return GetValueByIndex(static_cast<TDataType*>(pSource),GetComponentIndex());
}

/// Add the variable to the Kratos Registry
void Register() const {
const std::string variable_path("variables.all."+Name());
if (!Registry::HasItem(variable_path)) {
Registry::AddItem<VariableType>(variable_path, *this);
Registry::AddItem<VariableType>("variables."+Registry::GetCurrentSource()+"."+Name(), *this);
} else if(!Registry::GetItem(variable_path).IsSameType(*this)) {
KRATOS_ERROR << "Attempting to register " << Name()
<< " but a variable with the same name and different type already exists"
<< std::endl;
}
}
Comment on lines +321 to +331
Copy link
Member

Choose a reason for hiding this comment

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

I am curious, since registering the same variable from multiple apps causes some seg fault at the moment, shoudn't we throw an error if it exists, rather than checking the 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 think that Kratos should be able to support registering the same variable from multiple applications, as long as the type is the same. Using the registry in this way would be a step in that direction, since users of the registry are sure to always get the same thing when retrieving a given variable (something that is not so clear with components if Kratos allows overwriting variables as is happening now).


///@}
///@name Access
///@{
Expand Down Expand Up @@ -471,13 +476,6 @@ class Variable : public VariableData
return *static_cast<const TDataType*>(pValue + index);
}

void RegisterThisVariable(){
std::string variable_path = "variables.all." + Name();
if(!Registry::HasItem(variable_path)){
Registry::AddItem<VariableType>(variable_path, *this);
}
}

///@}
///@name Serialization
///@{
Expand Down
3 changes: 2 additions & 1 deletion kratos/includes/define.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,8 @@ catch(...) { Block KRATOS_THROW_ERROR(std::runtime_error, "Unknown error", MoreI
#endif
#define KRATOS_REGISTER_VARIABLE(name) \
AddKratosComponent(name.Name(), name); \
KratosComponents<VariableData>::Add(name.Name(), name);
KratosComponents<VariableData>::Add(name.Name(), name); \
name.Register();

#ifdef KRATOS_REGISTER_3D_VARIABLE_WITH_COMPONENTS
#undef KRATOS_REGISTER_3D_VARIABLE_WITH_COMPONENTS
Expand Down
4 changes: 3 additions & 1 deletion kratos/includes/kratos_application.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ class KRATOS_API(KRATOS_CORE) KratosApplication {
/// Destructor.
virtual ~KratosApplication()
{
// This must be commented until tests have been fixed.
DeregisterVariables();
DeregisterCommonComponents();
DeregisterApplication();
}
Expand All @@ -148,6 +148,8 @@ class KRATOS_API(KRATOS_CORE) KratosApplication {

void RegisterKratosCore();

void DeregisterVariables();

template<class TComponentsContainer>
void DeregisterComponent(std::string const & rComponentName);

Expand Down
14 changes: 12 additions & 2 deletions kratos/includes/registry_item.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
#include <boost/any.hpp>
#else
#include <any>
#endif
#endif

// External includes

Expand Down Expand Up @@ -257,7 +257,7 @@ class KRATOS_API(KRATOS_CORE) RegistryItem
KRATOS_CATCH("");
}

template<typename TDataType, typename TCastType>
template<typename TDataType, typename TCastType>
TCastType const& GetValueAs() const
{
KRATOS_TRY
Expand Down Expand Up @@ -285,6 +285,16 @@ class KRATOS_API(KRATOS_CORE) RegistryItem

bool HasItem(std::string const& rItemName) const;

template<class OtherType>
bool IsSameType(const OtherType& rOther) const {
try {
GetValue<OtherType>();
} catch (...) {
return false;
}
return true;
}

///@}
///@name Input and output
///@{
Expand Down
14 changes: 14 additions & 0 deletions kratos/sources/kratos_application.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,20 @@ void KratosApplication::RegisterKratosCore() {
RegisterVoxelMesherOperation();
}


void KratosApplication::DeregisterVariables() {
const std::string path = "variables."+mApplicationName;
if (Registry::HasItem(path)) {
auto& r_variables = Registry::GetItem(path);
// Iterate over items at path. For each item, remove it from the mappers.all branch too
for (auto i_key = r_variables.KeyConstBegin(); i_key != r_variables.KeyConstEnd(); ++i_key) {
Registry::RemoveItem("variables.all."+*i_key);
}
Registry::RemoveItem(path);
}
}
Comment on lines +354 to +364
Copy link
Member

@sunethwarna sunethwarna Jul 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 removes the variables registered by the application from the all, hence we don't allow registering same variable from multiple apps. So, I would as mentioned in the earlier comment, throw an error if the same variable name is registered from multiple places.

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 note that the scope of this PR is very narrow (it is intended only for testing, it is not a general "unload application" mechanism). What I am doing here is just ensuring that whoever registered the variable takes care of de-registering it, not ensuring that the kernel remains in a complete state after application de-registry. I understand that this is consistent with what the de-registration logic does for other types of objects.



template<class TComponentsContainer>
void KratosApplication::DeregisterComponent(std::string const & rComponentName) {
auto path = std::string(rComponentName)+"."+mApplicationName;
Expand Down
18 changes: 18 additions & 0 deletions kratos/tests/cpp_tests/containers/test_variables.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,25 @@ KRATOS_TEST_CASE_IN_SUITE(VariablesKeyOrder, KratosCoreFastSuite) {
}
}

KRATOS_TEST_CASE_IN_SUITE(VariablesRegister, KratosCoreFastSuite) {
Variable<double> new_var("NEW_DUMMY_VARIABLE");
new_var.Register();
KRATOS_EXPECT_TRUE(Registry::HasItem("variables.all.NEW_DUMMY_VARIABLE"));
KRATOS_EXPECT_TRUE(Registry::HasItem("variables.KratosMultiphysics.NEW_DUMMY_VARIABLE"));

// Attempting to register a variable with the same name and type does nothing
Variable<double> duplicate_variable("NEW_DUMMY_VARIABLE");
duplicate_variable.Register();
KRATOS_EXPECT_TRUE(Registry::HasItem("variables.all.NEW_DUMMY_VARIABLE"));
KRATOS_EXPECT_TRUE(Registry::HasItem("variables.KratosMultiphysics.NEW_DUMMY_VARIABLE"));

// Attempting to register a variable with the same name and different type is an error
Variable<int> wrong_type_variable("NEW_DUMMY_VARIABLE");
KRATOS_EXPECT_EXCEPTION_IS_THROWN(
wrong_type_variable.Register(),
"Attempting to register NEW_DUMMY_VARIABLE but a variable with the same name and different type already exists"
)
}


}
Expand Down
19 changes: 12 additions & 7 deletions kratos/tests/cpp_tests/sources/test_registry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ KRATOS_TEST_CASE_IN_SUITE(RegistryDataValue, KratosCoreFastSuite)
{
double value = 3.14;
RegistryItem value_registry_item("value_item", value);

KRATOS_EXPECT_EQ(value_registry_item.Name(),"value_item");
KRATOS_EXPECT_TRUE(value_registry_item.HasValue());
KRATOS_EXPECT_FALSE(value_registry_item.HasItems());
Expand All @@ -115,7 +115,7 @@ KRATOS_TEST_CASE_IN_SUITE(RegistryJsonValue, KratosCoreFastSuite)
std::string value_item_json = R"({
"value_item": "3.14"
})";

KRATOS_EXPECT_EQ(value_registry_item.ToJson(), value_item_json);
}

Expand All @@ -125,7 +125,7 @@ KRATOS_TEST_CASE_IN_SUITE(RegistryJsonSubValue, KratosCoreFastSuite)
RegistryItem registry_item("items");

registry_item.AddItem<double>("sub_value_item", value);

KRATOS_EXPECT_FALSE(registry_item.HasValue());
KRATOS_EXPECT_TRUE(registry_item.HasItems());
KRATOS_EXPECT_FALSE(registry_item.HasItem("test"));
Expand All @@ -138,15 +138,15 @@ KRATOS_TEST_CASE_IN_SUITE(RegistrySubValue, KratosCoreFastSuite)
{
double value = 3.14;
RegistryItem registry_item("items");

registry_item.AddItem<double>("sub_value_item", value);
auto& sub_item = registry_item.GetItem("sub_value_item");

KRATOS_EXPECT_EQ(sub_item.Name(),"sub_value_item");
KRATOS_EXPECT_TRUE(sub_item.HasValue());

registry_item.RemoveItem("sub_value_item");

KRATOS_EXPECT_FALSE(registry_item.HasItems());
KRATOS_EXPECT_FALSE(registry_item.HasItem("sub_value_item"));
}
Expand Down Expand Up @@ -309,6 +309,11 @@ KRATOS_TEST_CASE_IN_SUITE(RegistrySomeRegisteredVariables, KratosCoreFastSuite){
KRATOS_EXPECT_TRUE(Registry::HasItem("variables.all.DISPLACEMENT_Z"));
}

KRATOS_TEST_CASE_IN_SUITE(RegistryIsSameType, KratosCoreFastSuite) {
Variable<double> test_double("TEST_DOUBLE");
KRATOS_EXPECT_TRUE(Registry::GetItem("variables.all.TEMPERATURE").IsSameType(test_double));
KRATOS_EXPECT_FALSE(Registry::GetItem("variables.all.VELOCITY").IsSameType(test_double));
}

}
} // namespace Kratos.
Loading