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

Clean up a couple of friend declarations in CpGridData.hpp #761

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aritorto
Copy link
Member

Not relevant for the Reference Manual.

@aritorto
Copy link
Member Author

jenkins build this please

Copy link
Member

@blattms blattms left a comment

Choose a reason for hiding this comment

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

Unfortunately, the cleanup also seems to remove quite some part of the checks. Is that intended?

Normally refactoring should not change the amount of tests. Why is this different here? Might we miss problems due to this?

{
// Check if the element belongs to the leaf grid view or the level zero grid.
// leaf_to_level_cells_ [leaf idx] = {level where the entity was born, equivalent cell idx in that level}
if ((pgrid_ -> leaf_to_level_cells_.empty()) || (pgrid_-> level_to_leaf_cells_.empty())) // entity on a level grid, with level>0
Copy link
Member

Choose a reason for hiding this comment

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

Please use Entity<0>::isLeaf for the decision. Should be more readable

@aritorto
Copy link
Member Author

It's true that the clean up removed lines on the tests, however, those checked implementation details instead of features. For example, when we use "internally" -1 to denote that an entity does not verify certain property. Or when we check properties of children cells, it should be sufficient to use the hierarchic iterators instead of accessing via parent_to_children_cells_ . Sorry if "Clean up" on the title does not fully describe the PR intend

@blattms
Copy link
Member

blattms commented Sep 20, 2024

I see. Fair enough.

I see that we have the checks using child_to_parent as we are checking Entity::father.
But the checks the other way around (using parent_to_children) are removed. I think we should still have those, but refactored using HierarchicIterator. That should be possible, right?

I think moving more to the standard DUNE interface even in the tests is a very good change. Thanks a lot.

@aritorto
Copy link
Member Author

True, I haven't tried to use the iterators "in the other direction". That's a great idea. I'll draft this PR for now. Thanks for your feedback!

@aritorto aritorto marked this pull request as draft September 20, 2024 08:28
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