Flip fresh

[MohamedElkamash]

• Added FlipSidesetGenerator Class
• Added four tests
• Added documentation

[aprilnovak]

In this PR, it looks like you are trying to merge into my devel branch - but on the other PR, you specified my next branch as the target. Can you edit this PR to use next as the target? If not, just make a new PR which picks aprilnovak:next as the target.

[MohamedElkamash]

Done

[aprilnovak]

Looks good! I will review now

[aprilnovak]

@lindsayad sorry we do not need a review from you - Mohamed is my student and we are doing a practice PR here on my fork before doing a real PR in MOOSE.

[aprilnovak]

This should be a const reference (const BoundaryName &)

[MohamedElkamash]

Is there a specific reason for this?

[aprilnovak]

Yes, one reason is related to MOOSE's Controls system. That system allows an input file to essentially modify input file parameters on-the-fly -- so using a reference will make sure that everything in an input file is deeply connected to the input parameter.

It's also convention in MOOSE to use whenever possible, so when you cannot do const references there's some extra context that the software developer can have.

[aprilnovak]

Did you ever try that "neighbor through face" idea? Which would get this information from the mesh topology instead of looping through and checking each face as a candidate?

[MohamedElkamash]

Do you mean using a built in function that takes a neighbor and returns the side?

[aprilnovak]

Yes, I thought you had found a possible candidate - I'd prefer that if possible since it'd be a bit more readable code and would hopefully be faster (not a constraint from the test cases you have, but could be important for larger meshes).

[MohamedElkamash]

The only function I found that seemed to do something similar was: which_side_am_i (const Elem * e) which we discussed before and found that is not doing what I thought, other than this I didn't find a suitable candidate function

[aprilnovak]

Hmm, I am not sure - ok let's leave the code as-is for now, and we can ask on the MOOSE repo if they have any advice on where to look for something like that (if it exists).

[MohamedElkamash]

Can you check which_neighbor_am_i()
I think this may be the one, I didn't understand its documentation the first time but may be it the correct one
https://mooseframework.inl.gov/docs/doxygen/libmesh/classlibMesh_1_1Elem.html#ac4fd20bbdc7f49f25ecc67d67f2a2295

[aprilnovak]

I think that might be it (not 100% sure until we test it). It looks like these lines in which_neighbor_am_i look like they're doing the same looping-over-candidates you are doing (but it would be preferred if we can use the which_neighbor_am_i to reduce code duplication).

2326   for (unsigned int s=0, n_s = this->n_sides(); s != n_s; ++s)
 2327     if (this->[neighbor_ptr](s) == eparent)
 2328       return s;

Can you try it out?

[aprilnovak]

This should be expanded to get more specific - generally, you want the requirement to be unique for every test in the tests file (otherwise, strictly speaking, a test would be redundant with another test if it was entirely duplicative in what it is checking)

[aprilnovak]

Same comment applies to the other three tests as well

[aprilnovak]

Done with my review - when you make changes, you can just add another commit locally, and then push it to the same branch (flip_fresh). It will automatically update the github PR page here.

[lindsayad]

Haha no worries

[MohamedElkamash]

I committed the new changes and pushed them, do I need to submit another pull request?

[aprilnovak]

No, github knows to update this existing PR - if you look in the "Files changed" tab, you can verify that your recent changes from this morning are indeed there.