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

make_flux_sparsity_pattern() revision #14868

Merged
merged 3 commits into from Mar 13, 2023

Conversation

vyushut
Copy link
Contributor

@vyushut vyushut commented Mar 9, 2023

face_has_flux_coupling may be a predicate made of multiple, complicated checks; It's better to move it after the main logic the face loop; Moreover, face_has_flux_coupling may be impossible to run if, for example, the neighbor is not an active element;

…ed checks; It's better to move it after the main logic the face loop; Moreover, face_has_flux_coupling may be impossible to run o if, for example, the neighbor is not active element;
Copy link
Member

@tjhei tjhei left a comment

Choose a reason for hiding this comment

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

Yes, this makes a lot of sense to me: Why check the users' predicate if we do not consider this face anyways?

Can you please add a simple test that calls this function with a predicate function that prints cell indices of the cell and its neighbor to deallog?

(I am hoping somebody else can review the series of PRs Vladimir will open as we were working together)

…er which outputs to deallog. This should happen exactly once for every face.
* ---------------------------------------------------------------------
*/

// @sect3{Include files}
Copy link
Member

Choose a reason for hiding this comment

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

Replace this comment with a short description of what this test does.




// @sect3{The main() function}
Copy link
Member

Choose a reason for hiding this comment

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

remove these comments

dof_handler.distribute_dofs(finite_element);

// Compute the sparsity pattern specifying a face filter
DynamicSparsityPattern dsp(dof_handler.n_dofs(), dof_handler.n_dofs());
Copy link
Member

Choose a reason for hiding this comment

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

do you also want to print the pattern?

Triangulation<dim> triangulation;
make_two_elements(triangulation);

deallog << "dealii::Sparsity" << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

can you remove this line?

const unsigned int face_index)
{
bool on_OY = (std::abs(cell->face(face_index)->center()(0)) < 0.01);
deallog.get_file_stream()
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
deallog.get_file_stream()
deallog

deallog.get_file_stream()
<< "This sentence should appear once when the corresponding face is visited only once"
<< std::endl;
deallog.get_file_stream() << cell->index() << std::endl;
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
deallog.get_file_stream() << cell->index() << std::endl;
deallog << "cell index:" << cell->index() << std::endl;


// @sect3{Include files}

#include <deal.II/base/convergence_table.h>
Copy link
Member

Choose a reason for hiding this comment

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

can you go through and remove some of the unnecessary headers?

@vyushut
Copy link
Contributor Author

vyushut commented Mar 13, 2023

Timo,
Thanks for reviewing. I've made some changes to make the test more readable to anyone who will break it))

Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

Yes, nice work!

@bangerth bangerth merged commit 40c7656 into dealii:master Mar 13, 2023
@tjhei tjhei mentioned this pull request Mar 14, 2023
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

5 participants