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 1013 add missing api #323

Merged
merged 17 commits into from
Apr 22, 2024

Conversation

lucacarniato
Copy link
Contributor

No description provided.

Copy link
Contributor

@andreasbuykx andreasbuykx left a comment

Choose a reason for hiding this comment

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

I've had a look at the CLG class only....

/// @brief Computes the node from an index
[[nodiscard]] const Point& Node(const UInt index) const
{
const UInt n = static_cast<UInt>(std::floor(index / NumM()));
Copy link
Contributor

Choose a reason for hiding this comment

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

why use std::floor here? index/NumM() is integer division.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding index-to-curvilinear-pair and index-from-curvilinear-pair methods that provide the conversion.

Copy link
Contributor Author

@lucacarniato lucacarniato Apr 16, 2024

Choose a reason for hiding this comment

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

not sure it will be faster than:

    [[nodiscard]] const Point& Node(const UInt index) const
    {
        const UInt n = index / NumM();
        const UInt m = index % NumM();
        return m_gridNodes(n, m);
    }

data needs to be loaded into CPU caches. the approach above saves some memory

libs/MeshKernel/src/CurvilinearGrid/CurvilinearGrid.cpp Outdated Show resolved Hide resolved
libs/MeshKernel/src/CurvilinearGrid/CurvilinearGrid.cpp Outdated Show resolved Hide resolved
libs/MeshKernel/src/CurvilinearGrid/CurvilinearGrid.cpp Outdated Show resolved Hide resolved
@lucacarniato lucacarniato marked this pull request as ready for review April 17, 2024 09:17
Copy link
Contributor

@andreasbuykx andreasbuykx left a comment

Choose a reason for hiding this comment

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

Again I checked the CurvilinearGrid class only.
Nice work. I think some additional unit tests (esp. the const vs. non-const GetNode behavior and the GetNodeIndex methods) should be added.

const UInt n = index / NumM();
const UInt m = index % NumM();
return {n, m};
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

can you add some unit tests to verify/demonstrate the convention?

Comment on lines +47 to +49
/// @brief Typedef for edge indices
using CurvilinearEdgeNodeIndices = std::array<CurvilinearGridNodeIndices, 2>;

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use the Edge type, defined in Entities.hpp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is possible. But it requires a few other changes in other parts that deserve a separate pr

Comment on lines 327 to 328
/// @param[in] boundingBox The bounding box
void BuildTree(Location meshLocation, const BoundingBox& boundingBox);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this function a little dangerous? We could pass a bounding box that is for a small part of the domain or even outside of the mesh domain, the tree would be empty, and many subsequent operations that used the tree would be invalid

Comment on lines +376 to +380
bool m_nodesRTreeRequiresUpdate = true; ///< m_nodesRTree requires an update
bool m_edgesRTreeRequiresUpdate = true; ///< m_edgesRTree requires an update
bool m_facesRTreeRequiresUpdate = true; ///< m_facesRTree requires an update
std::unordered_map<Location, std::unique_ptr<RTreeBase>> m_RTrees; ///< The RTrees to use
BoundingBox m_boundingBoxCache; ///< Caches the last bounding box used for selecting the locations
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be an idea to remove the trees from the curvilinear mesh completely?
Have functions that can create trees that have been pass as parameters.
The the algorithms that need the functionality provided by the tree can defined thier own trees and use them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a separate task for this

Comment on lines 239 to 252
{
BuildTree(Location::Nodes);
SearchNearestLocation(point, Location::Nodes);
if (GetNumLocations(Location::Nodes) == 0)
if (rtree->GetQueryResultSize() <= 0)
{
return {constants::missing::uintValue, constants::missing::uintValue};
throw AlgorithmError("Query result size <= 0.");
}

const auto nodeIndex = GetLocationsIndices(0, Location::Nodes);
return m_gridIndices[nodeIndex];
return rtree->GetQueryResult(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the missing::uIntValue be returned here rather than raise an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

often the return value is used as an index, an exception is better

@lucacarniato lucacarniato merged commit 510322b into master Apr 22, 2024
12 of 13 checks passed
@lucacarniato lucacarniato deleted the feature/GRIDEDIT-1013_add_missing_api branch April 22, 2024 15:19
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.

None yet

3 participants