Refactor Tet::choose_diagonal()

[nmnobre]

Hi,

This still doesn't resolve the main issue, but removes the uncertainty from nodal permutations out of the equation...

Cheers,
-N

[moosebuild]

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

Coverage

	047a4c	#4117 6e238c
	Total	Total	+/-	New
Rate	63.32%	63.32%	-0.00%	100.00%
Hits	74333	74331	-2	3
Misses	43051	43051	-	0

Diff coverage report

Full coverage report

This comment will be updated on new commits.

[nmnobre]

Right, I can't really decide if this useful or not. permute_elements() is clearly a debugging/testing utility only (and in fairness, this is clearly stated in its docstring). So, can it even happen that the elements in the same mesh are locally numbered differently? If the user manually permutes their mesh before it's ingested, sure, but that's unlikely - though the user would probably still expect the same results in that case. Are there any other circumstances?

[jwpeterson]

I guess you deleted the comment (?), but I ran intro_ex4 with your suggested modifications and the solution looks like this:

Do you see weird artifacts on the sides of the cube which are supposed to be identically zero?

Yeah, I think I see some non-zero values on the sides... is that what you were referring to?

[nmnobre]

I guess you deleted the comment (?)

I did.

Yeah, I think I see some non-zero values on the sides... is that what you were referring to?

It was.

But this is not a good test because permute_elements() does nothing to fix boundary ids after the permutation.
And if you disable the permutations, everything looks good. Thus my recent comment about the possible uselessness of this PR.

[jwpeterson]

Thus my recent comment about the possible uselessness of this PR.

What was your motivation in creating the PR? Just because we can't easily recreate the scenario in a unit test doesn't mean that the change is useless...

I guess you deleted the comment (?)

I did.

OK, well it would be easier for me (and anyone viewing this PR in the future!) to follow along with what's going on if you leave the comment history intact (maybe use strikethrough text or something) and avoid force-pushing the branch while it's under discussion.

[nmnobre]

What was your motivation in creating the PR? Just because we can't easily recreate the scenario in a unit test doesn't mean that the change is useless...

In a sentence: I'm not sure our tetrahedral refinement is trustworthy. I started playing with vector fe ex4, and noticed that the solution would be all weird with uniform_refine(2) and started digging... @roystgnr suggested we tried permuting elements to see if you'd still get the same results, but we don't, even with uniform_refine(0), because of the way we select the diagonal (in case multiple diagonals are the same length, it's a coin flip as to whether which gets selected - this PR removes this uncertainty).

OK, well it would be easier for me (and anyone viewing this PR in the future!) to follow along with what's going on if you leave the comment history intact (maybe use strikethrough text or something) and avoid force-pushing the branch while it's under discussion.

Yes, you're right, I was hoping you hadn't seen the comment yet, I apologise.

BUT, this PR has already revealed something at least, choosing a different diagonal is causing the MOOSE non-conformal tests to fail - and I have no idea why, because the new nodes are exactly the same. But it feels like progress - or worst case scenario, the MOOSE checks are not robust.

[nmnobre]

But it feels like progress - or worst case scenario, the MOOSE checks are not robust.

Yep, that was MOOSE: idaholab/moose#30196.

[roystgnr]

permute_elements() does nothing to fix boundary ids after the permutation.

This is true, but it wouldn't be hard to fix. We'd need to pass an optional BoundaryInfo * into Elem::permute(), the way we do into Elem::flip(), but I didn't avoid that because it was a bad idea, I just avoided it because it wasn't necessary for the new unit tests I was writing.

I'm not sure this PR is useful (there's always going to be some grid resolution where floating point error works out differently at different places, and instead of getting to a tie we can disambiguate lexicographically, we get to an almost-tie that goes one way or another), but I'm still stubbornly wondering if "permute elements after changing the diagonal selection" can teach us more - we wouldn't want to make a more sweeping change permanent, but selecting diagonals based on something like only physical point ordering might still help you with debugging.

[nmnobre]

I'm not sure this PR is useful (there's always going to be some grid resolution where floating point error works out differently at different places, and instead of getting to a tie we can disambiguate lexicographically, we get to an almost-tie that goes one way or another),

Yes, of course. The usefulness of this, if any, comes only from pure node reorderings. With this, you get the same mesh, no matter how you count. That's all. For instance:
https://github.com/idaholab/moose/blob/09b3c83c02189c5d90355d400588060dc48d2d21/framework/src/meshgenerators/MeshDiagnosticsGenerator.C#L1272-L1274

but I'm still stubbornly wondering if "permute elements after changing the diagonal selection" can teach us more

I'm not sure I understand, sorry.

[nmnobre]

We've decided spending the extra effort on the tie-break is probably not worth it, so this PR is now just a very minor refactor.