Hiding Adapt Options From the Users

[mortezah]

This is achieved by making the return value of configure APIs const. This way normal users cannot change the options after the call to configure. Advanced users can get a modifiable Input pointer by calling ma::makeAdvanced() API and then modify the options manually, if they choose to. All the adapt APIs will work with both const Input* and Input* objects.

Todo:

• [x] clean up of test (i.e., some of them really don't need to change the input)

[jacobmerson]

@cwsmith can we rely on having c++11? Really what we want here is a move. The move constructor could just swap the pointers and then the deletion issue that @mortezah mentioned is no longer an issue.

[cwsmith]

@jacobmerson Sure. I'm fine with requiring C++11. If we do go that route, can we make a separate PR for the changes?

[jacobmerson]

@mortezah are you comfortable with move semantics? If not I can write up the prototype.

[mortezah]

@jacobmerson if I understand all I need to do is to have a default move constror in Input class. Then in makeAdvance I can just return std::move(in). Is that what you where thinking?

@cwsmith why do we need a new PR?

[cwsmith]

@mortezah I'd prefer not to embed the c++ standard requirement change in a series of commits related to meshadapt. Also, I suspect we can enable the c++11 requirement with a few edits in our CMake files, quickly test with a few downstream applications (mfem, etc.), then merge to develop/master.

[mortezah]

@mortezah I'd prefer not to embed the c++ standard requirement change in a series of commits related to meshadapt. Also, I suspect we can enable the c++11 requirement with a few edits in our CMake files, quickly test with a few downstream applications (mfem, etc.), then merge to develop/master.

So you want this branch be merged without using move semantics. And then another branch/PR where we add the move semantics?

[cwsmith]

We'd merge develop/master back into this branch so we can use the c++11 features.

I could make the c++11 change tonight if you guys are almost done with this work.

[mortezah]

We'd merge develop/master back into this branch so we can use the c++11 features.

I could make the c++11 change tonight if you guys are almost done with this work.

@cwsmith Yes. I think this is done. Once the c++ 11 features are in place on the develop branch, let me know and I will pull them here and use std::move in makeAdvanced.

[cwsmith]

OK. I'll work on this now.

[cwsmith]

@mortezah @jacobmerson the Cxx11 pr has been merged into develop. develop can be merged into this branch.

[cwsmith]

@mortezah When zoltan is disabled the meshadapt ctests

aniso_ma_serial, aniso_ma_serial_log_interpolation, and ma_serial

fail with the following output:

MeshAdapt input error:
core is not compiled with Zoltan. Use a different balancer or compile core with ENABLE_ZOLTAN=ON!

(detailed log https://github.com/SCOREC/core/pull/347/checks?check_run_id=3053420768#step:8:86)

aniso_ma_test.cc is the source for the two aniso_ma_serial* tests and requires pre-zoltan balancing:

https://github.com/SCOREC/core/blob/d828f504329a55fa05b01e6f9874cf4737d2e088/test/aniso_ma_test.cc#L57

ma_test.cc is the source for ma_serial and requires pre-zoltan here:

https://github.com/SCOREC/core/blob/d828f504329a55fa05b01e6f9874cf4737d2e088/test/ma_test.cc#L58

This PR https://github.com/mortezah/core/pull/3 adds/moves the ENABLE_ZOLTAN conditional to prevent these tests from running when zoltan is disabled.

Note, since some of tests are serial we could check if the process count was one and then disable pre-zoltan balancing. Given that these are not utilities we would expect a user to run it seemed like it was not worth the added complexity.

[mortezah]

@cwsmith I think all those tests can be run with parma without any problem. I will have to double check that. If that's the case I will remove the run pre Zoltan from them during the cleanup of this PR. Otherwise I will pull in the changes in the PR you mentioned above.

[mortezah]

@cwsmith can we rely on having c++11? Really what we want here is a move. The move constructor could just swap the pointers and then the deletion issue that @mortezah mentioned is no longer an issue.

@jacobmerson I don't seem to be able to get this to work with std::move. I have tried a few things:

1. moving the input to makeAdvanced (which is of type const ma::Input* and returning it. Compiler error of not being able to move a const to a non-const.
2. Having the two pointer sizeField and solutionTransfer be moved inside the constructor using the std::move instead of just assigning the pointers. That gives me the same problem as before when I delete the original input in makeAdvanced.

Am I missing anything, here?

[mortezah]

@jacobmerson I think your suggestion of using move is not going to work here, since other classes used in ma::Input, namely SizeField and SolutionTransfer, do not have proper copy constructors implemented so calling std::swap to swap those pointers inside move constructor would not fix the issue with deleting the in object inside makeAdvanced after getting a new one without the const qualifier.

If there is no other input on this, I will switch to my earlier solution (in which I remove the contents of the destructor of the Input class and add them at the end of adapt routines). This is not a clean solution but since all of this happens inside adapt, it should not cause too many problems.

In my opinion, any other way of cleaning up this code would require much more in-depth cleanup in lots of other places.

[cwsmith]

@mortezah Is this ready for review?

[mortezah]

@mortezah Is this ready for review?

Yes.

[mortezah]

@cwsmith I will take a look at this and let you know.

[cwsmith]

@mortezah Thank you. We are all set with Albany (albany wiki page). The only remaining application that needs investigation is m3dc1.

[mortezah]

@mortezah Thank you. We are all set with Albany (albany wiki page). The only remaining application that needs investigation is m3dc1.

@cwsmith

I will take care of m3dc. You can mark it as done here.

Also, I have committed the fix for the leak in maLayerCoarsen.cc. But I haven't been able to test it using address-sanitizer. That seems to require gcc10. Do we have access to that at SCOREC?

[cwsmith]

@mortezah Thank you. I marked m3dc1 as done.

The following commands will setup an environment on SCOREC Rhel7 using gcc10:

# Set Environment for Rhel-7 systems, gcc 10
module unuse /opt/scorec/spack/lmod/linux-rhel7-x86_64/Core 
module use /opt/scorec/spack/v0154_2/lmod/linux-rhel7-x86_64/Core 
#the zoltan module automatically loads parmetis since it is a dependency of zoltan
module load \
gcc/10.1.0 \
mpich \
zoltan \
cmake/3.20.0  \
simmetrix-simmodsuite/17.0-210808dev \
simmetrix/simModeler/11.0-210808-dev

[mortezah]

@cwsmith I have confirmed that my latest commit fixes the leak for vtxElmMixedBalance.

There are a couple of other leaks that I know how to fix. Do you want me to fix those on this PR or separately on a new one?

         57 - test_residual_error_estimate (Failed) - leaks from apf::getElement, getElementData, createMeshElement
        120 - highOrderSolutionTransfer (Failed) - leak from iterator end not being called ?
        125 - crack_test (Failed) - leaks from mds calls

[cwsmith]

Sounds good. Thank you.

I think a separate commit for the other leaks would make more sense since I assume they are not related to the changes in this PR.

[cwsmith]

@mortezah I think we are ready to merge. Are there any other changes to make here?

[mortezah]

@mortezah I think we are ready to merge. Are there any other changes to make here?

@cwsmith I don't think so.