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

Convert a few more locally-owned loops with filters. #13073

Merged
merged 2 commits into from Jan 19, 2022

Conversation

bangerth
Copy link
Member

Another follow-up to #12952.

/rebuild

IteratorFilters::LocallyOwnedCell())
{
(void)cell;
++n_my_cells;
Copy link
Member Author

Choose a reason for hiding this comment

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

It is disappointing that one has to give the loop variable a name, but that appears to be the case based on the standard.

Copy link
Member

Choose a reason for hiding this comment

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

Would it be more elegant to use std::count_if here (and below)?

Copy link
Member

Choose a reason for hiding this comment

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

Or use what you did above in source/distributed/repartitioning_policy_tools.cc.

const auto locally_owned_cells =
    tria_in.active_cell_iterators() | IteratorFilters::LocallyOwnedCell();
const unsigned int n_locally_owned_active_cells =
    std::distance(locally_owned_cells.begin(), locally_owned_cells.end());

for (const auto &cell : this->active_cell_iterators() |
IteratorFilters::SubdomainEqualTo(my_subdomain))
{
(void)cell;
Copy link
Member

Choose a reason for hiding this comment

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

see above

@@ -22,6 +22,7 @@
#include <deal.II/distributed/p4est_wrappers.h>
#include <deal.II/distributed/tria.h>

#include <deal.II/grid/filtered_iterator.h>
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't look like you need it here?

Suggested change
#include <deal.II/grid/filtered_iterator.h>

for (const auto &cell : this->cell_iterators() |
IteratorFilters::LocallyOwnedLevelCell())
{
(void)cell;
Copy link
Member

Choose a reason for hiding this comment

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

see above

IteratorFilters::LocallyOwnedCell())
{
(void)cell;
++n_my_cells;
Copy link
Member

Choose a reason for hiding this comment

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

Or use what you did above in source/distributed/repartitioning_policy_tools.cc.

const auto locally_owned_cells =
    tria_in.active_cell_iterators() | IteratorFilters::LocallyOwnedCell();
const unsigned int n_locally_owned_active_cells =
    std::distance(locally_owned_cells.begin(), locally_owned_cells.end());

@@ -156,10 +156,10 @@ namespace GridTools
typename Triangulation<dim, spacedim>::active_cell_iterator>>
boxes;
boxes.reserve(tria->n_active_cells());
for (const auto &cell : tria->active_cell_iterators())
for (const auto &cell : tria->active_cell_iterators() |
IteratorFilters::LocallyOwnedCell())
if (cell->is_locally_owned())
Copy link
Member

Choose a reason for hiding this comment

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

You don't need this anymore I guess.

Suggested change
if (cell->is_locally_owned())

@bangerth
Copy link
Member Author

Ah yes, std::count_if is a clever solution. Thanks for pointing that out!

Copy link
Member

@jppelteret jppelteret left a comment

Choose a reason for hiding this comment

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

This is pretty cool :-)

@@ -19,6 +19,7 @@

#include <deal.II/distributed/fully_distributed_tria.h>

#include <deal.II/grid/filtered_iterator.h>
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look as if IteratorFilters are used in this file?

// It is very unlikely that a single process has more than 2 billion
// cells, but we might as well check.
// It is very unlikely that a single process has more than 2
// billion cells, but we might as well check.
Copy link
Member

Choose a reason for hiding this comment

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

If you're going to push "billon" onto the next line, then maybe it makes sense to move the number "2" as well.

@bangerth
Copy link
Member Author

Adjusted as requested!

@bangerth
Copy link
Member Author

Seems to work now!

@marcfehling You put the block on this PR. Want to take another look?

@bangerth
Copy link
Member Author

bangerth commented Jan 4, 2022

Ping?

@bangerth
Copy link
Member Author

Ping? (@marcfehling had the last set of comments that created a hold on the PR.)

Copy link
Member

@marcfehling marcfehling left a comment

Choose a reason for hiding this comment

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

Sorry for the late feedback. Forgot about this open PR over the holidays.

Comment on lines 209 to 212
const auto locally_owned_cells =
tria_in.active_cell_iterators() | IteratorFilters::LocallyOwnedCell();
const unsigned int n_locally_owned_active_cells =
std::distance(locally_owned_cells.begin(), locally_owned_cells.end());
Copy link
Member

Choose a reason for hiding this comment

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

If you want, you can use std::count_if here, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, so done!

++n_locally_owned_active_cells;
const unsigned int n_locally_owned_active_cells =
std::count_if(tria_in.begin_active(),
tria_in.end(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tria_in.end(),
typename Triangulation<dim, spacedim>::active_cell_iterator(
tria_in.end()),

This should fix the compiler problem

/home/runner/work/dealii/dealii/source/distributed/repartitioning_policy_tools.cc:210:20: note:   deduced conflicting types for parameter ‘_IIter’ (‘dealii::TriaActiveIterator<dealii::CellAccessor<1, 1> >’ and ‘dealii::TriaIterator<dealii::CellAccessor<1, 1> >’)

@marcfehling marcfehling merged commit fcbb8b6 into dealii:master Jan 19, 2022
@bangerth bangerth deleted the filter branch January 19, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants