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

Add SymmetryTransformerGenerator, with plane symmetry only currently #21718

Merged

Conversation

RocksSavage
Copy link
Contributor

@RocksSavage RocksSavage commented Jul 28, 2022

Beware, the option to optionally use StitchedMeshGenerator has not been tested. Tests have not been written.
I would appreciate it if the test currently in simple.i would end up in the documentation as an example of what
default parameters are ran when the stitch option is exercised.

  • add tests
  • add stitching capability
  • document extent of support in md file

Refs #21578

@GiudGiud GiudGiud changed the title This is an incomplete commit. Establishes SymmetryTransformerGenerator. Add SymmetryTransformerGenerator, with plane symmetry only currently Aug 3, 2022
@GiudGiud GiudGiud self-assigned this Aug 3, 2022
@GiudGiud GiudGiud force-pushed the create-SymmetryTransformerGenerator branch from 492bbba to 38e3ef1 Compare November 2, 2022 19:58
@GiudGiud
Copy link
Contributor

GiudGiud commented Nov 2, 2022

This is on hold until we can get better fixing of negative volumes in libmesh

@GiudGiud GiudGiud force-pushed the create-SymmetryTransformerGenerator branch 3 times, most recently from bfc5caf to a67e406 Compare November 23, 2022 20:58
@GiudGiud
Copy link
Contributor

This will be near-ready on the next libmesh submodule update.

@GiudGiud GiudGiud force-pushed the create-SymmetryTransformerGenerator branch from a67e406 to 924264b Compare November 25, 2022 15:21
@GiudGiud GiudGiud force-pushed the create-SymmetryTransformerGenerator branch from 924264b to 23ee722 Compare January 2, 2023 23:27
@GiudGiud
Copy link
Contributor

GiudGiud commented Jan 2, 2023

@loganharbour this should be ready for review please

@GiudGiud
Copy link
Contributor

GiudGiud commented Jan 2, 2023

nevermind still no libmesh update

RocksSavage and others added 2 commits January 10, 2023 15:47
Beware, the option to optionally use StitchedMeshGenerator has not been tested. Tests have not been written.
I would appreciate it if the test currently in simple.i would end up in the documentation as an example of what
default parameters are ran when the stitch option is exercised.
- change to Transform instead of Transformer
- add parameter name for stitching
- less docs on issue of normalizing vector and just tell user what is happening
refs idaholab#21578
@GiudGiud GiudGiud force-pushed the create-SymmetryTransformerGenerator branch from 23ee722 to bc9c9f8 Compare January 10, 2023 23:26
@GiudGiud
Copy link
Contributor

@loganharbour ready for review.

I've had to add a bit of lower quality code (const_cast) to allow for a sub-generator to run after a main generator.
If that's not agreeable, then I reckon I want to delete the stitching and get this merged.

@GiudGiud GiudGiud force-pushed the create-SymmetryTransformerGenerator branch from bc9c9f8 to 2f17d47 Compare January 11, 2023 00:07
@moosebuild
Copy link
Contributor

moosebuild commented Jan 11, 2023

Job Documentation on 917e4f8 wanted to post the following:

View the site here

This comment will be updated on new commits.

@GiudGiud GiudGiud force-pushed the create-SymmetryTransformerGenerator branch from 2f17d47 to 0b2bbdf Compare January 11, 2023 02:09
@moosebuild
Copy link
Contributor

moosebuild commented Jan 11, 2023

Job Coverage on 917e4f8 wanted to post the following:

Framework coverage

e4507b #21718 917e4f
Total Total +/- New
Rate 84.53% 84.54% +0.01% 87.50%
Hits 81696 81735 +39 35
Misses 14946 14947 +1 5

Diff coverage report

Full coverage report

Modules coverage

Coverage did not change

Full coverage reports

Reports

Warnings

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

This comment will be updated on new commits.

Copy link
Member

@loganharbour loganharbour left a comment

Choose a reason for hiding this comment

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

I'm really against the subgenerator for a few reasons:

  1. The implementation of the before action is poor. This is resolvable and thus is minor.
  2. I think that it will be confusing if we allow the addition of subgens like this. We could end up in a position where we could end up with a wonky graph that isn't intuitive to debug.
  3. The action of the stitching is so simple and only requires copying one input to another in another MG. I think it's a poor use of the subgenerator system. It makes it look like we're okay with having a ton of meta generators - which I think is a poor idea because the best part of the MG system is its pluggability, and having to maintain all of that meta interaction sounds awful.

@moosebuild
Copy link
Contributor

All jobs on 48111e4 : invalidated by @GiudGiud

@GiudGiud
Copy link
Contributor

Should be good now

@GiudGiud GiudGiud force-pushed the create-SymmetryTransformerGenerator branch from 48111e4 to 76756c6 Compare January 12, 2023 02:07
- Remove stitching from symmetry generator
- more constness
- use paramError over mooseError when possible
@loganharbour loganharbour merged commit 88eb118 into idaholab:next Jan 19, 2023
@GiudGiud
Copy link
Contributor

Thanks for the contribution @RocksSavage !
I hope all is well in Utah

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

4 participants