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

Feature/gridedit 1548 casulli refinement depths #390

Open
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

BillSenior
Copy link
Contributor

No description provided.

Added api unit functions and unit test, though the test does not yet work correctly
Added missing file and updated cmake file
…mkernel_mesh2d_set_property. Must also pass a propertyId to indicate which property is being defined
@BillSenior BillSenior force-pushed the feature/GRIDEDIT-1548_casulli_refinement_depths branch from 52fde8f to c38c831 Compare December 16, 2024 16:47
Comment on lines +47 to +49

double searchRadiusSquared = 1.0e5;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where did this number (1e5) come from?

Comment on lines +103 to +105

/// @brief Determine the size of the edge-length vector required
int Size(const MeshKernelState& state, const meshkernel::Location location) const override;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update comment, interpolation is not only for edges

Comment on lines +132 to +134
// The current property id, initialised with a number larger than the Mesh2D:::Property enum values
static int currentPropertyId = static_cast<int>(meshkernel::Mesh2D::Property::EdgeLength);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update comment, its no longer larger than the enum.

Comment on lines +139 to +153
std::map<int, std::unique_ptr<PropertyCalculator>> allocatePropertyCalculators()
{
std::map<int, std::unique_ptr<PropertyCalculator>> propertyMap;

int propertyId = static_cast<int>(meshkernel::Mesh2D::Property::Orthogonality);
propertyMap.emplace(propertyId, std::make_unique<OrthogonalityPropertyCalculator>());

propertyId = static_cast<int>(meshkernel::Mesh2D::Property::EdgeLength);
propertyMap.emplace(propertyId, std::make_unique<EdgeLengthPropertyCalculator>());

return propertyMap;
}

/// @brief Map of property calculators, from an property identifier to the calculator.
static std::map<int, std::unique_ptr<PropertyCalculator>> propertyCalculators = allocatePropertyCalculators();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could this be implemented as a singlton rather than a static global variable

Comment on lines +72 to +76

/// @brief Set sample data
// template <meshkernel::ValidConstDoubleArray VectorType>
// void SetData(const int propertyId, const VectorType& sampleData) override;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove commented out code

Comment on lines +55 to +57
/// @note saves only triangle and quadrilateral elements.
void PrintVtk(const std::vector<Point>& nodes, const std::vector<std::vector<UInt>>& faces, const std::string& fileName);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rename to save vtk

Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to preserve this method?

Comment on lines +296 to +298
{
refineNode = true;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could break here inside if

Comment on lines +796 to +797
if (node1 == constants::missing::uintValue || node2 == constants::missing::uintValue)
// if (node1 == constants::missing::uintValue && node2 == constants::missing::uintValue)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove commented out code

Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented code

@@ -32,7 +32,9 @@
#include <vector>

#include "MeshKernel/Mesh2D.hpp"
#include "MeshKernel/Parameters.hpp"
#include "MeshKernel/Polygons.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

polygons.hpp unused header

@@ -112,4 +113,70 @@ namespace meshkernel
/// @brief Get the string representation of the CurvilinearDirection enumeration values.
const std::string& CurvilinearDirectionToString(CurvilinearDirection direction);

/// @brief The concept specifies that the array type must have an access operator returning the array element type or can be converted to one
template <typename ArrayType, typename ResultType>
concept ArrayConstAccessConcept = requires(const ArrayType& array, const size_t i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

when do you need these concepts?


/// @brief A simple bounded array
template <const UInt Dimension>
class BoundedArray
Copy link
Contributor

Choose a reason for hiding this comment

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

if the dimension in known at compile time, can we use an std::array ?

bool PointIsInElement(const Point& pnt, const UInt faceId) const;

/// @brief Print the mesh graph
void Print(std::ostream& out = std::cout) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

i suggest removing this function in the final commit

@@ -94,6 +120,25 @@ namespace meshkernel
/// @param [in, out] nodeMask Node mask information
static void InitialiseFaceNodes(const Mesh2D& mesh, std::vector<NodeMask>& nodeMask);

/// @brief INitialise the node mask using depth data
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Comment on lines +55 to +57
/// @note saves only triangle and quadrilateral elements.
void PrintVtk(const std::vector<Point>& nodes, const std::vector<std::vector<UInt>>& faces, const std::string& fileName);

Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to preserve this method?

Comment on lines +796 to +797
if (node1 == constants::missing::uintValue || node2 == constants::missing::uintValue)
// if (node1 == constants::missing::uintValue && node2 == constants::missing::uintValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented code

{
return faceId;
}
else
Copy link
Contributor

Choose a reason for hiding this comment

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

remove else

{
m_elementCentreRTree->SearchNearestPoint(pnt);

if (m_elementCentreRTree->HasQueryResults())
Copy link
Contributor

Choose a reason for hiding this comment

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

consider early returning instead

40, 39, 40, 25, 33,
35, 27, 36, 29, 31,
30, 37, 38, 32, 40};
// std::vector<int> expectedEdgesStart = {9, 25, 9, 12, 13,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented code

Comment on lines +215 to +219
/// @brief The minimum depth considered when computing refinement based on depths
///
/// Only depths greater than this value will be considered.
double minimum_refinement_depth = 0.0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pass as parameter and remove from the struct here.
This way the api does not change

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

Successfully merging this pull request may close these issues.

2 participants