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.

Sign up for GitHub

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

Jump to bottom

Split lower-d mesh blocks based on side types #29144

Merged
merged 1 commit into from
Nov 28, 2024
Merged

Split lower-d mesh blocks based on side types #29144

merged 1 commit into from
Nov 28, 2024

Conversation

YaqiWang
Copy link
Contributor

Closes #29142. Without this fix, the new test 3d-prism.i will error out in --mesh-only mode.

@YaqiWang
Copy link
Contributor Author

This will need a Griffin patch. Tag @NamjaeChoi

@YaqiWang YaqiWang force-pushed the lower_d_mesh_fix_29142 branch from e60c6fd to 2b5693c Compare November 26, 2024 04:25
@moosebuild
Copy link
Contributor

Job Precheck on 2b5693c : invalidated by @YaqiWang

@YaqiWang YaqiWang force-pushed the lower_d_mesh_fix_29142 branch from 2b5693c to 29ae513 Compare November 26, 2024 05:29
@moosebuild
Copy link
Contributor

moosebuild commented Nov 26, 2024 •
edited
Loading

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

View the site here

This comment will be updated on new commits.

@YaqiWang YaqiWang force-pushed the lower_d_mesh_fix_29142 branch from 29ae513 to 9408cfb Compare November 26, 2024 08:09
GiudGiud
GiudGiud reviewed Nov 26, 2024
View reviewed changes
Copy link
Contributor

@GiudGiud GiudGiud left a comment

Choose a reason for hiding this comment

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

Can we use contains instead of count and empty() instead of size() ?
Using a fixed subdomain Id was likely an optimization so we should try to stay optimized

@YaqiWang
Copy link
Contributor Author

Can we use contains instead of count and empty() instead of size() ? Using a fixed subdomain Id was likely an optimization so we should try to stay optimized

The number of lower-dimensional blocks depends on a mesh. It is not fixed, but on the other hand, it is not large. I guess we do not have other ways, unless we change exodus output in libMesh?

@GiudGiud
Copy link
Contributor

I guess we do not have other ways, unless we change exodus output in libMesh?

I m requesting an inconsequential change here imo, it s just performance (potentially)

@YaqiWang YaqiWang force-pushed the lower_d_mesh_fix_29142 branch from 9408cfb to e215b4c Compare November 26, 2024 18:41
@YaqiWang YaqiWang force-pushed the lower_d_mesh_fix_29142 branch 2 times, most recently from cdbcd2d to e3a08cf Compare November 26, 2024 20:05
@moosebuild
Copy link
Contributor

moosebuild commented Nov 26, 2024 •
edited
Loading

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

Framework coverage

cbffa0 #29144 cbd987
Total Total +/- New
Rate 85.14% 85.13% -0.00% 74.80%
Hits 107525 107589 +64 95
Misses 18772 18787 +15 32

Diff coverage report

Full coverage report

Modules coverage

Coverage did not change

Full coverage reports

Reports

Warnings

  • framework new line coverage rate 74.80% is less than the suggested 90.0%

This comment will be updated on new commits.

@YaqiWang YaqiWang force-pushed the lower_d_mesh_fix_29142 branch from e3a08cf to 5e05ad8 Compare November 26, 2024 23:06
@YaqiWang
Copy link
Contributor Author

@GiudGiud More comments?

@GiudGiud GiudGiud self-assigned this Nov 27, 2024
@YaqiWang YaqiWang force-pushed the lower_d_mesh_fix_29142 branch from 5e05ad8 to 683c680 Compare November 27, 2024 04:06
@YaqiWang YaqiWang force-pushed the lower_d_mesh_fix_29142 branch 2 times, most recently from 795555c to a6a28b6 Compare November 27, 2024 16:15
@idaholab idaholab deleted a comment from moosebuild Nov 27, 2024
GiudGiud
GiudGiud approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@GiudGiud GiudGiud left a comment

Choose a reason for hiding this comment

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

We are trading one issue (same block name different element types) for another here (broken block restriction of lower D variables for the kernels checked in the framework) but they will both be resolved in time

@YaqiWang YaqiWang force-pushed the lower_d_mesh_fix_29142 branch from a6a28b6 to cbd9872 Compare November 27, 2024 16:43
@loganharbour loganharbour merged commit b751bf3 into idaholab:next Nov 28, 2024
49 of 51 checks passed
@YaqiWang YaqiWang mentioned this pull request Nov 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GiudGiud GiudGiud GiudGiud approved these changes

@lindsayad lindsayad Awaiting requested review from lindsayad lindsayad is a code owner

@roystgnr roystgnr Awaiting requested review from roystgnr roystgnr is a code owner

Assignees

@GiudGiud GiudGiud

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MooseMesh::buildLowerDMesh needs to add separate blocks for lower-d elements in different types
4 participants
@YaqiWang @moosebuild @GiudGiud @loganharbour