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

Use std::optional to reduce code duplication in boundary condition setting #3434

Merged
merged 37 commits into from
Sep 26, 2024

Conversation

garth-wells
Copy link
Member

@garth-wells garth-wells commented Sep 24, 2024

See #3413.

Also:

  • Simplifies syntax, especially from C++.
  • Now clearer what the Dirichet BC 'set' function does.
  • Improves documentation.
  • Changes the confusingly named 'scale' argument to 'alpha` in the function that modifies an array due to boundary conditions.

@garth-wells garth-wells added enhancement New feature or request housekeeping Tidying and style improvements labels Sep 24, 2024
@garth-wells garth-wells added the documentation Documentation improvements or fixes label Sep 25, 2024
Copy link
Member

@jhale jhale left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! std::optional is great for us, we should carefully roll it out everywhere.

std::span<T> b(_b.mutable_array());
std::ranges::fill(b, 0.0);
std::span b(_b.mutable_array());
std::ranges::fill(b, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Tangential, but do we have a set style on this? Personally I prefer using floating point literals even if a cast from an integer literal is possible. We are also using T(1) below.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think our house (0.0) style pre-dates supporting different types. I prefer

std::ranges::fill(b, 0);

to

std::ranges::fill(b, T(0));

because it's less syntax. I think

std::ranges::fill(b, 0.0);

is possibly misleading since T is not necessarily double.

cpp/dolfinx/fem/DirichletBC.h Show resolved Hide resolved
python/dolfinx/fem/bcs.py Outdated Show resolved Hide resolved
python/dolfinx/fem/bcs.py Show resolved Hide resolved
@@ -188,7 +188,7 @@ def action_A(x, y):
y.scatter_reverse(la.InsertMode.add)

# Set BC dofs to zero
fem.set_bc(y.array, [bc], scale=0.0)
bc.set(y.array, alpha=0.0)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This is a nice way of homogenising the bc, which is super convenient for optimization.

Copy link
Member Author

@garth-wells garth-wells Sep 26, 2024

Choose a reason for hiding this comment

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

We already have a function that can do this more 'directly'

/// @todo Review this function - it is almost identical to the
, but it had quite a bit of duplicated code.

Copy link
Member Author

@garth-wells garth-wells Sep 26, 2024

Choose a reason for hiding this comment

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

I've added an optimisation in DirichletBC::set for when alpha == 0, and remove the function that was largely a duplicate.

@garth-wells garth-wells added this pull request to the merge queue Sep 26, 2024
Merged via the queue into main with commit e41b0b1 Sep 26, 2024
27 checks passed
@garth-wells garth-wells deleted the garth/bc-optional branch September 26, 2024 15:13
jorgensd added a commit to jorgensd/dolfinx_mpc that referenced this pull request Sep 27, 2024
jorgensd added a commit to jorgensd/dolfinx_mpc that referenced this pull request Sep 27, 2024
jorgensd added a commit to jorgensd/dolfinx-tutorial that referenced this pull request Sep 27, 2024
jorgensd added a commit to jorgensd/dolfinx-tutorial that referenced this pull request Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation improvements or fixes enhancement New feature or request housekeeping Tidying and style improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants