Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

For QCM, add parameters to control quadrature rule and number of quadrature points #1432

Merged
merged 11 commits into from
Mar 1, 2025

Conversation

voferreira
Copy link
Collaborator

@voferreira voferreira commented Feb 26, 2025

Description

The QCM void fraction scheme in unresolved CFD-DEM used the Gauss rule to distribute the quadrature points over the cell. The number of quadrature points was fixed at 2 per dimension. In mesh independence studies, in which we want to preserve the sphere size across meshes, this could leave gaps between the QCM virtual spheres. The void fraction calculation would not account for particles in this uncovered region.

For this reason, we decided to improve the method's flexibility by adding two user-defined parameters, namely quadrature rule and n quadrature points. The first controls the quadrature rule to be applied upon distributing the quadrature points, while the second allows for choosing the number of quadrature points per dimension. In this implementation, the available rules are Gauss and Gauss-Lobatto. We chose these two because the first respects the default of the solver and the latter places quadrature points at the boundaries of the cells.

Testing

All previous tests pass. One new test was added to track the feature.

Documentation

The parameters were documented accordingly.

Miscellaneous (will be removed when merged)

Checklist (will be removed when merged)

See this page for more information about the pull request process.

Code related list:

  • All in-code documentation related to this PR is up to date (Doxygen format)
  • Copyright headers are present and up to date
  • Lethe documentation is up to date
  • New feature has unit test(s) (preferred) or application test(s), and restart files are in the generator folder
  • The branch is rebased onto master
  • Changelog (CHANGELOG.md) is up to date
  • Code is indented with indent-all and .prm files (examples and tests) with prm-indent

Pull request related list:

  • Labels are applied
  • There are at least 2 reviewers (or 1 if small feature) excluding the responsible for the merge
  • If this PR closes an issue or is related to a project, it is linked in the "Projects" or "Development" section
  • If the fix is temporary, an issue is opened
  • The PR description is cleaned and ready for merge

@voferreira voferreira added the WIP When a PR is open but not ready for review label Feb 26, 2025
@blaisb
Copy link
Contributor

blaisb commented Feb 27, 2025

@voferreira can you please revert the changes on the files that are not related?

@voferreira voferreira added the Enhancement New feature or request label Feb 27, 2025
@voferreira voferreira added Ready for review and removed WIP When a PR is open but not ready for review labels Feb 27, 2025
@brunaccampos brunaccampos self-requested a review February 27, 2025 16:23
Copy link
Contributor

@blaisb blaisb left a comment

Choose a reason for hiding this comment

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

Few comments there and there.

# SPDX-FileCopyrightText: Copyright (c) 2021-2024 The Lethe Authors
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception OR LGPL-2.1-or-later

# Listing of Parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you indicate as a comment which generator is used to generate the restart file for this problem
Could you also add a comment regarding what this test actually tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this description fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

yup

# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception OR LGPL-2.1-or-later

# Listing of Parameters
#----------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as for the other prm file.

Comment on lines 146 to 147
else
quadrature = std::make_shared<QGauss<dim>>(fe->degree + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This else should never happen no, so why not just throw instead? we shuold not reach that point.

@@ -72,6 +78,8 @@ namespace Parameters
unsigned int particle_refinement_factor;
double qcm_sphere_diameter;
bool qcm_sphere_equal_cell_volume;
VoidFractionQCMRule qcm_quadrature_rule;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not make it specific to QCM even though it is in a way. Let's just apply it everywhere and call that VoidFractionQuadratureRule
We could eventually have a general parameter called quadraturerule if we change quadrature for the other physics. Maybe now is the time to start it cleanly.

Comment on lines 144 to 145
quadrature = std::make_shared<QGaussLobatto<dim>>(
this->void_fraction_parameters->qcm_n_quadrature_points);
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to assert that the number of quadrature point is positive
Or what you could do is if it's -1 you put degree+1 and if it's a positive number you use that number.
Here nothing prevents you from putting a negative value.

{
// Ensure coherence with default
if (qcm_n_quadrature_points == -1)
qcm_n_quadrature_points = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be 2, but should be void_fraction_degree + 1.

{
// Ensure coherence with default
if (qcm_n_quadrature_points == -1)
qcm_n_quadrature_points = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

fe_degree +2

Copy link
Collaborator

@OGaboriault OGaboriault left a comment

Choose a reason for hiding this comment

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

Tony comments. Good work

if (this->void_fraction_parameters->quadrature_rule ==
Parameters::VoidFractionQuadratureRule::gauss)
{
if (this->void_fraction_parameters->n_quadrature_points == -1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

here it could be 0

if (this->void_fraction_parameters->quadrature_rule ==
Parameters::VoidFractionQuadratureRule::gauss_lobatto)
{
if (this->void_fraction_parameters->n_quadrature_points == -1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

here could be 0

"Choose which quadrature rule to follow when distributing quadrature points for the QCM void fraction scheme");
prm.declare_entry(
"n quadrature points",
"-1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be zero here

Copy link
Collaborator

@brunaccampos brunaccampos left a comment

Choose a reason for hiding this comment

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

Just very minor suggestions

Co-authored-by: Bruna Campos <76260238+brunaccampos@users.noreply.github.com>
Copy link
Contributor

@blaisb blaisb left a comment

Choose a reason for hiding this comment

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

Very good.

# SPDX-FileCopyrightText: Copyright (c) 2021-2024 The Lethe Authors
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception OR LGPL-2.1-or-later

# Listing of Parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

yup

@blaisb blaisb merged commit 9d066a2 into master Mar 1, 2025
11 checks passed
@blaisb blaisb deleted the qcm_gauss_lobatto branch March 1, 2025 18:38
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants