Skip to content

Default block restriction #30300

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

Open
wants to merge 13 commits into
base: next
Choose a base branch
from

Conversation

ChengHauYang
Copy link

@ChengHauYang ChengHauYang commented Apr 10, 2025

close #30299

Reason

When using element subdomain modifiers (e.g., additive manufacturing, welding), simulations are often restricted to a specific subset of blocks (subdomains). However, users currently need to manually specify these block restrictions repeatedly in multiple places within the input file, such as kernel_coverage_block_list, material_coverage_block_list, and the block parameter for individual Kernels, Materials, and Variables.

This repetitive setup increases the chances of user error and makes input files harder to maintain. A more user-friendly and centralized way to control block restrictions is desired.

Design

This PR introduces a new parameter default_block under the [Problem] block to provide a centralized way to control block restrictions throughout the input file.

[Problem]
  default_block = '0 1 3'
[]

Setting default_block will automatically:

(a) Set kernel_coverage_check = ONLY_LIST and kernel_coverage_block_list = '0 1 3' to [Problem].

(b) Set material_coverage_check = ONLY_LIST and material_coverage_block_list = '0 1 3' to [Problem].

(c) Apply the specified blocks to any BlockRestrictable object (e.g., Kernels, Materials, Variables) that does not explicitly define its own block parameter.

Flexibility & Overriding Behavior
(a) Users can still override kernel_coverage_block_list or material_coverage_block_list inside [Problem] even if default_block is set.

(b) Users can explicitly set block for individual Kernels, Materials, or Variables, provided that the specified blocks are a subset of default_block.

(c) If a user tries to specify blocks outside of default_block, an warning will be raised to prevent unintended behavior.

Example:

[Kernels]
  [./diff]
    type = HeatConduction
    variable = cond
  [../]
[]

This will automatically apply to blocks 0 1 3.

But if user set:

[Kernels]
  [./diff]
    type = HeatConduction
    variable = cond
    block = '0 1 2 3' # block 2 is not in default_block
  [../]
[]

Will show a warning:

*** Warning ***
The supplied block list is not a subset of the default block list

Impact

This change improves usability and reduces duplication in input file setup, especially for simulations involving subdomain-based modifiers. It provides a clear and centralized control for block restrictions while preserving flexibility for advanced users.

This enhancement is particularly useful for applications like additive manufacturing, and welding.

… (`modules/heat_transfer/test/tests/default_block_check`) (c) material_coverage_check fixing
Copy link
Contributor

@hugary1995 hugary1995 left a comment

Choose a reason for hiding this comment

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

Looks good overall. Some minor comments.

Copy link
Member

@lindsayad lindsayad left a comment

Choose a reason for hiding this comment

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

We'll need some markdown documentation for this. It will help me understand the context more. I always want to carefully understand the need any time we're adding a parameter to our kitchen sink object (FEProblemBase)

@lindsayad
Copy link
Member

Your PR description is nice and thorough, so a good start candidate for translation into markdown

@lindsayad
Copy link
Member

Most importantly, thank you for contributing!

…d` to explain the `default_block` effect in coverage checking and block restrictions. (b) move the tests into `/test/tests/default_block_check`. (c) Fix according to Gary's comments.
@ChengHauYang
Copy link
Author

Dear @hugary1995 and @lindsayad, thanks for your code review. I fixed the code according to Gary's comments and prepared some markdown documentation, adding them into sanity_checking.md and BlockRestrictable.md. Thanks

Copy link
Contributor

@hugary1995 hugary1995 left a comment

Choose a reason for hiding this comment

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

Just a few more minor comments then it's all clear from my side.

@hugary1995 hugary1995 self-assigned this Apr 11, 2025
Copy link
Contributor

@hugary1995 hugary1995 left a comment

Choose a reason for hiding this comment

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

I approve. We should have at least one approve from the framework team. @lindsayad

@moosebuild
Copy link
Contributor

moosebuild commented Apr 11, 2025

Job Documentation, step Docs: sync website on ec5a868 wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

moosebuild commented Apr 11, 2025

Job Coverage, step Generate coverage on ec5a868 wanted to post the following:

Framework coverage

Inconsistent report tags were found between the head and base reports.
This can happen when reports are missing from either the head or the base.

Inconsistent tags:
framework-torch-gpu
Full coverage report

Modules coverage

Fluid properties

788dee #30300 ec5a86
Total Total +/- New
Rate 86.08% 86.09% +0.01% -
Hits 7982 7983 +1 0
Misses 1291 1290 -1 0

Diff coverage report

Full coverage report

Full coverage reports

Reports

This comment will be updated on new commits.

Comment on lines 10 to 12
this behavior by explicitly specifying the `block` parameter within each object. If an explicitly
specified `block` is not a subset of the `default_block`, MOOSE will throw a warning
to notify the user about the inconsistency.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we want this to be a warning. I think one possible use case for this @hugary1995 mentioned is excluding lower-dimensional subdomains. But in that case, any lagrange multipliers rightly living on the lower-d subdomains will trigger this warning

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Also semantically it isn't a mistake to override default.

Copy link
Author

Choose a reason for hiding this comment

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

Dear @lindsayad and @hugary1995, thanks for your feedback. I removed the warning and associated function and test in the new commit. Thanks!

…te the associated function and regression test.
@ChengHauYang
Copy link
Author

ChengHauYang commented Apr 12, 2025

In the new commit, I tried to address:

(a) The system shall throw an error if the user specifies either kernel_coverage_check or material_coverage_check together with default_block.
(b) The system shall allow users to include ANY_BLOCK_ID in the default_block parameter.
(c) Avoiding unnecessary copying of the block_param vector by using a reference.

Thanks for the feedback!

…nel_coverage_check` or `material_coverage_check` together with `default_block`. (b) The system shall allow users to include `ANY_BLOCK_ID` in the `default_block` parameter. (c) Avoiding unnecessary copying of the `block_param` vector by using a reference.
@ChengHauYang
Copy link
Author

Dear @lindsayad,

I fixed the codes based on your feedback. Thanks.

With the discussions with @hugary1995 and his suggestions. We tried to have the below logic as well (which is aligned with your feedback):
If the user sets either kernel_coverage_check or material_coverage_check to ONLY_LIST or SKIP_LIST, and simultaneously provides a kernel_coverage_block_list or material_coverage_block_list, the presence of a default_block can introduce ambiguity in determining which blocks are subject to coverage checks. To avoid confusion, the system will throw an error in this case.
This restriction is necessary to prevent unintended behavior. However, there are situations where the user may want to completely disable kernel_coverage_check (kernel_coverage_check = FALSE) while still using default_block. In such cases, the system will not throw any error.

Copy link
Member

@lindsayad lindsayad left a comment

Choose a reason for hiding this comment

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

Almost there!

…d be devoid of implementation details (test-driven development)
@ChengHauYang
Copy link
Author

Dear @lindsayad,

Thanks for your constructive and detailed feedback. I have fixed the codes.

@moosebuild
Copy link
Contributor

Job Conda build (ARM Mac) on ec5a868 : invalidated by @lindsayad

@hugary1995 hugary1995 requested a review from bwspenc April 15, 2025 15:24
@hugary1995
Copy link
Contributor

Let's wait for @bwspenc 's review.

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.

Default block restriction
4 participants