search

idaholab / moose

Multiphysics Object Oriented Simulation Environment
https://www.mooseframework.org
GNU Lesser General Public License v2.1
1.71k stars 1.04k forks source link

Some MeshGenerator tests need exodiff to be pedantic #13346

Open veeshy opened 5 years ago

veeshy commented 5 years ago

Bug Description

Mesh generators testing for making sidesets and nodesets will pass tests in a false positive manner. These tests should run in the harness with exodiff_opts = '-pedantic' so exodiff reports different meshes and the test fails.

Steps to Reproduce

Take moose/test/tests/meshgenerators/sidesets_between_subdomains_generator/sideset_between_subdomains.i and comment out the central_boundary block which is what is being tested, run the test and it passes!

I'm not sure how many of the MeshGenerator tests would fail this way, but likely worth it to add the exodiff_opts to all of those tests.

Impact

Make some MeshGenerator tests actually test the things they say they are testing.

permcody commented 4 years ago

Hmm - finally ran into this. It's unclear if -pedantic will cause other issues, but something does need to be done to catch these issues.

pbehne commented 6 months ago

I just ran into this in #26252 as I was refactoring several sideset-related meshgenerators. Basically, I had a bug that resulted in meshes with different sidesets from what was golded, yet Exodiff only prints a warning and doesn't fail without -pedantic`. Would it be much more computationally expensive to have pedantic on by default?

permcody commented 6 months ago

I don't believe you can turn -pedantic on by default because we allow different node/element numbering for many of our cases and that can happen when you are running with replicated versus distributed mesh and don't have options to force a specific node numbering. That being said, for the mesh generator cases, it likely is appropriate and perhaps should be enforced...

It has been several years since we've looked at using pedantic so maybe the landscape is a little different. I'm open to you giving it a try and reporting back. Make sure you run with distributed mesh and replicated mesh and in parallel on a few tests and let us know. Maybe we can come up with a reasonable middle ground to catch as many of these cases as we can.

lindsayad commented 6 months ago

It seems like the issue is with exodiff. From a user perspective, node and element numbering should be "hidden" and not relevant to them (although we do support some bad behavior with postprocessing based on specified ids). However, sideset ids and names are user-set. The former should really not be considered in a -pedantic test from a MOOSE perspective while the latter should be. Perhaps the thing to do would be add something like a boundary restricted aux kernel to tests of these mesh generators that ascertain the sideset is being added to the correct side of interfaces. This aux kernel would populate the aux var with something non-zero when considering aux var dofs on the correct side of the interface. Then a "pedantic" check of the sideset ID would be encoded in the aux var field

pbehne commented 5 months ago

It seems like the issue is with exodiff. From a user perspective, node and element numbering should be "hidden" and not relevant to them (although we do support some bad behavior with postprocessing based on specified ids). However, sideset ids and names are user-set. The former should really not be considered in a -pedantic test from a MOOSE perspective while the latter should be. Perhaps the thing to do would be add something like a boundary restricted aux kernel to tests of these mesh generators that ascertain the sideset is being added to the correct side of interfaces. This aux kernel would populate the aux var with something non-zero when considering aux var dofs on the correct side of the interface. Then a "pedantic" check of the sideset ID would be encoded in the aux var field

This is clever. I can think of a potential downside: when adding new tests the mess with sidesets, the developer will have to remember to add this boundary restricted aux kernel to their test input. If we can come up with a way to detect whether this check should be added to a test, maybe we could automatically add the aux kernel within MOOSE without changing the input. I don't know how we would automatically determine how to do this, though. Maybe we could add a flag to one of the base meshgenerators that keeps track of whether we are messing with nodesets and sidesets?

lindsayad commented 5 months ago

Yea I don't think there's a great way to automate this. Even a flag requires manual specification in the input. I think it would be alright to handle these mesh generator tests manually, even knowing that if somebody adds new tests they may not remember to add these aux-var/aux-kernel objects