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

Enable Quadratic Elems in XYDelaunay and Related MGs #27546

Merged
merged 6 commits into from
May 24, 2024

Conversation

miaoyinb
Copy link
Contributor

@miaoyinb miaoyinb commented May 3, 2024

closes #27545

Reason

Design

Impact

@miaoyinb
Copy link
Contributor Author

miaoyinb commented May 3, 2024

@eshemon @roystgnr FYI

@moosebuild
Copy link
Contributor

moosebuild commented May 3, 2024

Job Documentation on c27f819 wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

moosebuild commented May 3, 2024

Job Coverage on c27f819 wanted to post the following:

Framework coverage

ef92d3 #27546 c27f81
Total Total +/- New
Rate 85.10% 85.11% +0.00% 96.08%
Hits 103329 103378 +49 49
Misses 18088 18090 +2 2

Diff coverage report

Full coverage report

Modules coverage

Reactor

ef92d3 #27546 c27f81
Total Total +/- New
Rate 93.93% 93.95% +0.02% 100.00%
Hits 6190 6197 +7 6
Misses 400 399 -1 0

Diff coverage report

Full coverage report

Full coverage reports

Reports

This comment will be updated on new commits.

@miaoyinb
Copy link
Contributor Author

miaoyinb commented May 6, 2024

This PR is expected to have conflicts with #27434 so I will fix after that one is merged.

@roystgnr Currently, four manual options (TRI3, TRI6, TRI7 and DEFAULT) are provided as you directed. If the generated elements and input elements do not have the same orders, it should be "okay" if we do not stitch the holes. But if holes are stitched:

  • If quadratic elements are set to be generated for linear input meshes. We can potentially convert all input meshes to second order.
  • If linear elements are set to be generated for quadratic input meshes. We can also potential convert all the input meshes to first order (may lose some information if the edges are curvy).
  • For mixed types of input meshes, we can also convert the incompatible part.
  • OR, we just throw error messages without conversion.

Any ideas/preferences would be appreciated.

@roystgnr
Copy link
Contributor

roystgnr commented May 6, 2024

We definitely don't want to automatically convert any higher-order inputs to first-order if they're going to be used for stitching; that's a good place to throw an error. It looks like we don't currently have any MOOSE MeshGenerator shim to libMesh::UnstructuredMesh::all_first_order(), but we could add one if someone really has a source mesh that's only higher-order and they want the first-order equivalent.

If higher-order meshes are only going to be used for boundary/hole definition ... that's the situation where I think we want to explicitly require users to specify TRI3 explicitly if they really want lower-order outputs.

And if we have mixed order between different inputs? My first instinct is that we should have a MeshGenerator shim to libMesh all_second_order() and all_complete_order(), but it wouldn't be completely crazy to add those as optional parameters in the base class instead, to be run following any generate(). I'd like to hear other opinions here.

@miaoyinb
Copy link
Contributor Author

miaoyinb commented May 9, 2024

Thanks, @roystgnr

I will add the error messages accordingly.

For all_first_order()/all_second_order()/all_complete_order(), do we prefer a separate MG (like ElementOrderConversionGenerator), or implementing them into the Mesh base class as options? @GiudGiud @loganharbour @lindsayad

@lindsayad
Copy link
Member

We already have second_order parameter in the mesh class. But I think I would personally prefer one ElementOrderConversionGenerator or multiple mesh generators (AllSecondOrderMeshGenerator, AllCompleteOrderMeshGenerator) as opposed to parameters on the MooseMesh class

@roystgnr
Copy link
Contributor

My vote would be for a single ConversionGenerator, with parameters to select the new order; that's just more flexible than the current mesh class parameter. We may have a mix of quadratic-capable and quadratic-incapable generators in an input, and in that case it might be useful to be able to select exactly where in the mesh generator tree we bump up the order - too early and the incapable generators can't handle it; too late and the capable generators need to all be mixed-order-input-capable too, which is a bit of an ask.

@GiudGiud
Copy link
Contributor

Same vote. An ElementOrderConversionGenetor with block-restriction will cover all our needs

@miaoyinb
Copy link
Contributor Author

Same vote. An ElementOrderConversionGenetor with block-restriction will cover all our needs

I am not sure if we want to do block restriction here. I guess a mixed-order mesh would be an issue in most cases.

@moosebuild
Copy link
Contributor

Job Precheck on 5cceb02 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/docs/PRs/27546/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 24e6ae5253b7318392e26426668de5fa1cee5f75

@moosebuild
Copy link
Contributor

Job Apptainer moose on 3f4bd39 : invalidated by @miaoyinb

@miaoyinb miaoyinb marked this pull request as ready for review May 16, 2024 02:24
@miaoyinb miaoyinb requested a review from lindsayad as a code owner May 16, 2024 02:24
@miaoyinb
Copy link
Contributor Author

@GiudGiud Just a friendly reminder. Your review would be greatly appreciated.

@lindsayad
Copy link
Member

@GiudGiud will be on travel. If he wants to remove himself from assignment, then we might review this faster

@miaoyinb
Copy link
Contributor Author

@GiudGiud will be on travel. If he wants to remove himself from assignment, then we might review this faster

Thanks @lindsayad . It would be up to @GiudGiud. I just want to make sure this PR is not forgotten.

@GiudGiud
Copy link
Contributor

Let's assume I can't get to it before I come back in 2 weeks. Not sure yet

@roystgnr
Copy link
Contributor

I'll review if @GiudGiud doesn't mind.

@GiudGiud
Copy link
Contributor

GiudGiud commented May 23, 2024 via email

Copy link
Contributor

@roystgnr roystgnr left a comment

Choose a reason for hiding this comment

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

I think we can clarify a few things in docs and error messages but that's arguable and this is a big improvement as-is.

framework/src/meshgenerators/XYDelaunayGenerator.C Outdated Show resolved Hide resolved
@miaoyinb
Copy link
Contributor Author

I will add more docs late today.

@roystgnr
Copy link
Contributor

Let me kick those failing tests; looks to me like they just happened to coincide with some planned github.inl.gov downtime.

Copy link
Contributor

@roystgnr roystgnr left a comment

Choose a reason for hiding this comment

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

I'd like to wait for those kicked tests to finish, or for anyone else who wants to chime in, but we should merge today if there aren't any surprises.

@lindsayad lindsayad merged commit 4409da0 into idaholab:next May 24, 2024
47 checks passed
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.

Enable Quadratic Elements in XYDelaunayGenerator and Related MGs
5 participants