Skip to content

refactor: limit magic paths use - #4102

Open
kdrienCG wants to merge 16 commits into
developfrom
refactor/kdrienCG/globalViewKeys
Open

refactor: limit magic paths use#4102
kdrienCG wants to merge 16 commits into
developfrom
refactor/kdrienCG/globalViewKeys

Conversation

@kdrienCG

@kdrienCG kdrienCG commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

This PR reduces the use of magic paths to access common objects for developers ("/Problem/domain/", "/Problem/domain/Constitutive", "/Problem/Events/", etc)

It introduces a ProblemRepository interface to provide a centralized definition of the retrieval method of the data root and of the main data owners (managers mainly).

It also adds a ProblemManagerBase lightweight interface over the ProblemManager that gives typed access to these objects without requiring to bring the full ProblemManager


Example of its use:

// Full path & explicit type way
PhysicsSolverManager & physicsSolverManager = this->getGroupByPath( "/Problem/Solvers" );

// Abstract 'Group' way ("invisible" dependancy):
Group & problemManager = this->getGroupByPath( "/Problem" );
Group & physicsSolverManager = problemManager.getGroup( "Solvers" );

is replaced by:

PhysicsSolverManager & physicsSolverManager = ProblemRepository::getManager< PhysicsSolverManager >( *this );

@kdrienCG
kdrienCG marked this pull request as ready for review July 23, 2026 15:37
@kdrienCG kdrienCG added ci: run integrated tests Allows to run the integrated tests in GEOS CI ci: run code coverage enables running of the code coverage CI jobs labels Jul 31, 2026
/**
* @struct GlobalViewKeys
*/
struct GlobalViewKeys

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.

Suggested change
struct GlobalViewKeys
struct ProblemViewKeys

struct GlobalViewKeys
{
/// @return Root problem group name
static constexpr char const * problem() { return "Problem"; }

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.

Suggested change
static constexpr char const * problem() { return "Problem"; }
static constexpr char const * problemManager() { return "Problem"; }

Comment on lines +30 to +32
/**
* @struct GlobalViewKeys
*/

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.

Suggested change
/**
* @struct GlobalViewKeys
*/
/**
* @brief All root data-repository view-keys of a given GEOS problem.
*/

Comment on lines +16 to +135
/**
* @file ProblemManagerBase.hpp
*/

#ifndef GEOS_DATAREPOSITORY_PROBLEMMANAGERBASE_HPP_
#define GEOS_DATAREPOSITORY_PROBLEMMANAGERBASE_HPP_

#include "dataRepository/Group.hpp"

namespace geos
{

class DomainPartition;
class EventManager;
class ExternalDataSourceManager;
class FieldSpecificationManager;
class FunctionManager;
class GeometricObjectManager;
class MeshManager;
class NumericalMethodsManager;
class OutputManager;
class PhysicsSolverManager;
class TasksManager;
namespace constitutive
{
class ConstitutiveManager;
}

namespace dataRepository
{


/**
* @class ProblemManagerBase
*/
class ProblemManagerBase : public Group
{
public:

using Group::Group;

virtual DomainPartition & getDomainPartition() = 0;
virtual DomainPartition const & getDomainPartition() const = 0;

virtual constitutive::ConstitutiveManager & getConstitutiveManager() = 0;
virtual constitutive::ConstitutiveManager const & getConstitutiveManager() const = 0;

virtual EventManager & getEventManager() = 0;
virtual EventManager const & getEventManager() const = 0;

virtual ExternalDataSourceManager & getExternalDataSourceManager() = 0;
virtual ExternalDataSourceManager const & getExternalDataSourceManager() const = 0;

virtual FieldSpecificationManager & getFieldSpecificationManager() = 0;
virtual FieldSpecificationManager const & getFieldSpecificationManager() const = 0;

virtual FunctionManager & getFunctionManager() = 0;
virtual FunctionManager const & getFunctionManager() const = 0;

virtual GeometricObjectManager & getGeometricObjectManager() = 0;
virtual GeometricObjectManager const & getGeometricObjectManager() const = 0;

virtual MeshManager & getMeshManager() = 0;
virtual MeshManager const & getMeshManager() const = 0;

virtual NumericalMethodsManager & getNumericalMethodsManager() = 0;
virtual NumericalMethodsManager const & getNumericalMethodsManager() const = 0;

virtual OutputManager & getOutputManager() = 0;
virtual OutputManager const & getOutputManager() const = 0;

virtual PhysicsSolverManager & getPhysicsSolverManager() = 0;
virtual PhysicsSolverManager const & getPhysicsSolverManager() const = 0;

virtual TasksManager & getTasksManager() = 0;
virtual TasksManager const & getTasksManager() const = 0;


virtual string const & getProblemName() const = 0;
virtual string const & getInputFileName() const = 0;
virtual string const & getRestartFileName() const = 0;
virtual string const & getSchemaFileName() const = 0;

};

/**
* @brief Gives the ProblemManagerBase from the given Group
* @param group The current Group in the Problem tree
* @return A reference to the ProblemManagerBase
*/
inline ProblemManagerBase & getProblemManagerBase( Group & group )
{
Group * current = &group;
while( current->hasParent() )
{
current = &current->getParent();
}
ProblemManagerBase * const root = dynamic_cast< ProblemManagerBase * >( current );
return *root;
}

/**
* @copydoc getProblemManagerBase( Group & )
*/
inline ProblemManagerBase const & getProblemManagerBase( Group const & group )
{
Group const * current = &group;
while( current->hasParent() )
{
current = &current->getParent();
}
ProblemManagerBase const * const root = dynamic_cast< ProblemManagerBase const * >( current );
return *root;
}

} /* namespace dataRepository */
} /* namespace geos */


#endif /* GEOS_DATAREPOSITORY_PROBLEMMANAGERBASE_HPP_ */

@MelReyCG MelReyCG Jul 31, 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.

I would like to propose a new version to go all the way with this approach:

Suggested change
/**
* @file ProblemManagerBase.hpp
*/
#ifndef GEOS_DATAREPOSITORY_PROBLEMMANAGERBASE_HPP_
#define GEOS_DATAREPOSITORY_PROBLEMMANAGERBASE_HPP_
#include "dataRepository/Group.hpp"
namespace geos
{
class DomainPartition;
class EventManager;
class ExternalDataSourceManager;
class FieldSpecificationManager;
class FunctionManager;
class GeometricObjectManager;
class MeshManager;
class NumericalMethodsManager;
class OutputManager;
class PhysicsSolverManager;
class TasksManager;
namespace constitutive
{
class ConstitutiveManager;
}
namespace dataRepository
{
/**
* @class ProblemManagerBase
*/
class ProblemManagerBase : public Group
{
public:
using Group::Group;
virtual DomainPartition & getDomainPartition() = 0;
virtual DomainPartition const & getDomainPartition() const = 0;
virtual constitutive::ConstitutiveManager & getConstitutiveManager() = 0;
virtual constitutive::ConstitutiveManager const & getConstitutiveManager() const = 0;
virtual EventManager & getEventManager() = 0;
virtual EventManager const & getEventManager() const = 0;
virtual ExternalDataSourceManager & getExternalDataSourceManager() = 0;
virtual ExternalDataSourceManager const & getExternalDataSourceManager() const = 0;
virtual FieldSpecificationManager & getFieldSpecificationManager() = 0;
virtual FieldSpecificationManager const & getFieldSpecificationManager() const = 0;
virtual FunctionManager & getFunctionManager() = 0;
virtual FunctionManager const & getFunctionManager() const = 0;
virtual GeometricObjectManager & getGeometricObjectManager() = 0;
virtual GeometricObjectManager const & getGeometricObjectManager() const = 0;
virtual MeshManager & getMeshManager() = 0;
virtual MeshManager const & getMeshManager() const = 0;
virtual NumericalMethodsManager & getNumericalMethodsManager() = 0;
virtual NumericalMethodsManager const & getNumericalMethodsManager() const = 0;
virtual OutputManager & getOutputManager() = 0;
virtual OutputManager const & getOutputManager() const = 0;
virtual PhysicsSolverManager & getPhysicsSolverManager() = 0;
virtual PhysicsSolverManager const & getPhysicsSolverManager() const = 0;
virtual TasksManager & getTasksManager() = 0;
virtual TasksManager const & getTasksManager() const = 0;
virtual string const & getProblemName() const = 0;
virtual string const & getInputFileName() const = 0;
virtual string const & getRestartFileName() const = 0;
virtual string const & getSchemaFileName() const = 0;
};
/**
* @brief Gives the ProblemManagerBase from the given Group
* @param group The current Group in the Problem tree
* @return A reference to the ProblemManagerBase
*/
inline ProblemManagerBase & getProblemManagerBase( Group & group )
{
Group * current = &group;
while( current->hasParent() )
{
current = &current->getParent();
}
ProblemManagerBase * const root = dynamic_cast< ProblemManagerBase * >( current );
return *root;
}
/**
* @copydoc getProblemManagerBase( Group & )
*/
inline ProblemManagerBase const & getProblemManagerBase( Group const & group )
{
Group const * current = &group;
while( current->hasParent() )
{
current = &current->getParent();
}
ProblemManagerBase const * const root = dynamic_cast< ProblemManagerBase const * >( current );
return *root;
}
} /* namespace dataRepository */
} /* namespace geos */
#endif /* GEOS_DATAREPOSITORY_PROBLEMMANAGERBASE_HPP_ */
/**
* @file IProblemRepository.hpp
*/
#ifndef GEOS_DATAREPOSITORY_IPROBLEMREPOSITORY_HPP_
#define GEOS_DATAREPOSITORY_IPROBLEMREPOSITORY_HPP_
#include "dataRepository/Group.hpp"
namespace geos
{
namespace dataRepository
{
/**
* @brief Gives the problem data repository interface from the given Group
* @param group The current Group in the Problem tree
* @return A reference to the problem data repository interface
*/
inline IProblemRepository & getProblemRepository( Group & group )
/**
* @copydoc getIProblemRepository( Group & )
*/
inline IProblemRepository const & getProblemRepository( Group const & group )
/**
* @brief Interface for an object which contain all the problem data-repository
*/
class IProblemRepository
{
public:
/**
* @brief Get a root data-repository object (a manager) of a given problem.
* - The consumer need to include the type definition
* - The interface implementation needs to implement a specialization for each types manager.
* @tparam ManagerType the type of the root data-repository object we want to get (DomainPartition, EventManager...)
* @return ManagerType& the root data-repository object instance reference
*/
template< typename ManagerType >
virtual ManagerType & getRootObject() = 0;
/**
* @brief Get a root data-repository object (a manager) of a given problem.
* - The consumer need to include the type definition
* - The interface implementation needs to implement a specialization for each types manager.
* @tparam ManagerType the type of the root data-repository object we want to get (DomainPartition, EventManager...)
* @return ManagerType const & the root data-repository object instance reference
*/
template< typename ManagerType >
virtual ManagerType const & getRootObject() const = 0;
// if an abstract Group getting method is absolutely needed, we can add:
//
// virtual Group & getManager( string_view managerKey ) = 0;
// virtual Group const & getManager( string_view managerKey ) const = 0;
//
// ... but ideally, we don't want to propose these to remove any "invisible" circular dependency practice.
};
/**
* @name Inline functions implementation
*/
///@{
inline IProblemRepository & getManagerRegistry( Group & group )
{
Group * current = &group;
while( current->hasParent() )
{
current = &current->getParent();
}
IProblemRepository * const root = dynamic_cast< IProblemRepository * >( current );
return *root;
}
inline IProblemRepository const & getIProblemRepository( Group const & group )
{
Group const * current = &group;
while( current->hasParent() )
{
current = &current->getParent();
}
IProblemRepository const * const root = dynamic_cast< IProblemRepository const * >( current );
return *root;
}
///@}
} /* namespace dataRepository */
} /* namespace geos */
#endif /* GEOS_DATAREPOSITORY_IPROBLEMREPOSITORY_HPP_ */

I push your almost-interface type approach a bit further, and I see the following benefits:

  • Each interface consumer need the declaration of the needed type,
  • Only need to add template specialisation for each possible getRootObject() type in ProblemManager.cpp,
  • No need to manipulate ProblemManager view-keys, only the type of the manager is needed (excepted for implementation, which I think is ideal),
  • We discourage a lot to create invisible circular dependancy
    • no need for forward declaration, explicit type will be used by consumer,
    • no abstract Group obtained by a name,
    • no assumption on the data-hierarchy structure.
  • We lock the mutability of the root-Group (unable to add sub-Groups to ProblemManager without being explicit with its type).

Last remarks:

  • If that does not compile (linkage) or if we don't want virtual method implemented in the ProblemManager package, we can implement that in a ProblemRepositoryImpl class (with its own cpp) and keep the interface approach (no Group inheritance/exposure).
  • I would let the commented versions to consolidate and explain the approach.

…ository, an ABC of ProblemManager

Each manager will then provide a global access method within the current problem
…ters for efficient look-up

- ConstitutiveManager is particular since it is owned by DomainPartition, the key index is given by it
- updated mesh-related views to use GroupKey
@MelReyCG
MelReyCG requested a review from andrea-borio as a code owner August 19, 2026 09:50
@MelReyCG MelReyCG added the ci: run CUDA builds Allows to triggers (costly) CUDA jobs label Aug 19, 2026
@MelReyCG
MelReyCG requested a review from herve-gross as a code owner August 19, 2026 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci: run code coverage enables running of the code coverage CI jobs ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: ready for review type: cleanup / refactor Non-functional change (NFC)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants