migration of mprans to xtensor

[JohanMabille]

Based on #1054 to trigger the CI. Both PRs will be merged in the end.

[codecov]

Codecov Report

Merging #1068 into master will decrease coverage by <.01%.
The diff coverage is 22%.

@@            Coverage Diff             @@
##           master    #1068      +/-   ##
==========================================
- Coverage   50.21%    50.2%   -0.01%     
==========================================
  Files         528      528              
  Lines      101550   101588      +38     
==========================================
+ Hits        50993    51004      +11     
- Misses      50557    50584      +27

Impacted Files	Coverage Δ
proteus/mprans/VOF.py	80.71% <100%> (+0.25%)	⬆️
proteus/NumericalSolution.py	49.25% <2.5%> (-0.95%)	⬇️
proteus/tests/test_boundaryconditions.py	97.57% <0%> (+0.12%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6762b5b...16329ff. Read the comment docs.

[JohanMabille]

@cekees @tridelat @zhang-alvin This PR is almost ready to go. I think it could be nice to merge it as soon as possible to avoid having a lot of conflicts, since we're changing a lot the APIs.

There is still an issue with the hdist build: the file RANS3PF takes too long to build, leading to a travis timeout. Splitting the file does not fix the problem. I am pretty sure that it is due to the high number of arguments of the functions exposed to the Python. Notice that the conda build is fine.

We have three options here: we can pass the hdist biuld as "allow to fail" until we have replaced the list of arguments with a dictionnary of arguments. Or I can start to implement the dictionnary of arguments in this PR and use it in this problematic file. Or I can revert the changes in RANS3PF and keep the cython implementation for now.

I prefer the first option since it allows this PR to get merged faster and avoid potential conflicts in the future. Let me know what you prefer.

[cekees]

@JohanMabille the main source of the long compile times is the c++ compile of our template instances at high optimization levels. On the hashdist side we turn off optimization by default in the setup file as per below. I think this was commented out for conda because we weren't carefully replacing the "-Wl,-O*" options, but I've fixed that below and tested that conda compiles proteus without problems (and quickly) on my laptop. I think generally we want the conda binary on conda forge to use optimization but turn it off and enable debugging of our travis builds.

    if type(value) == str:
        cfg_vars[key] = cfg_vars[key].replace("-Wstrict-prototypes", "")
        cfg_vars[key] = cfg_vars[key].replace("-Wall", "-w")
        cfg_vars[key] = cfg_vars[key].replace("-Wl,-O3","")
        cfg_vars[key] = cfg_vars[key].replace("-Wl,-O2","")
        cfg_vars[key] = cfg_vars[key].replace("-O3", "")
        cfg_vars[key] = cfg_vars[key].replace("-O2", "")
        cfg_vars[key] = cfg_vars[key].replace("-DNDEBUG", "-UNDEBUG")
        cfg_vars[key] = cfg_vars[key] + " -g"

[JohanMabille]

I will try to disable optimizations and see what it says, I agree that it would be more helpful to have the API with pyarray then that with raw arrays when migrating to the dict of numpy arrays.

EDIT: I think we should enable these optimizations again as soon as possible since some errors can be caught in Release mode only)

[cekees]

@JohanMabille note that you can't just uncomment the old setup modifications. You need to use what I posted above, particularly cfg_vars[key] = cfg_vars[key].replace("-Wl,-O3","") cfg_vars[key] = cfg_vars[key].replace("-Wl,-O2","")

Otherwise the conda build will likely fail.

[cekees]

@tridelat @zhang-alvin @ejtovar This PR swaps out cython wrapping for pybind11 and xtensor arrays for double*. As @JohanMabille mentioned, it doesn't yet reduce the size of the call signatures by directly passing the q, ebqe, etc. dict structures into C++, which is the objective. I do think the pybind11 and xtensor array approach is a signficant improvement as long as you guys can verify that we're not suffering a performance hit on some real problems. In this interim approach, to use a new array in C++ you'll still need to modify the .py, .h, and .cpp file of a model (the latter takes the place of the pyx file in cython), roughly the equivalent amount of your time as currently modifying .py, .h, and .pyx file. However, you can use multi-dimensional array indexing in c++ and other nice features of xtensor arrays. If are happy with this approach and can verify that you're ready to start using it moving forward, then I agree we should go ahead and merge this.

On the other hand, going the next step of replacing the multiple xtensor arrays with some kind of dictionary/dataframe would compete the full task but make the differences a bit larger. @SylvainCorlay could you update us on the status of xframe or similar (xrecord?) which we discussed early on? Our q, ebqe, etc., dictionaries are essentially named dataframes with the last (inner) dimension being variable (e.g. scalar, vector, tensor). I don't think that works with a pandas dataframe at present, though perhaps we could define our on vector and tensor types. Just directly using the existing dictionaries via pybind11 objects seems the simplest path forward.

[JohanMabille]

This is now ready to go!

[zhang-alvin]

Apologies for not looking into this sooner. I've recently completed the repository cleanup so all of the commits on the feature branch need to be rebased on-top of the revised master branch.

[SylvainCorlay]

Apologies for not looking into this sooner. I've recently completed the repository cleanup so all of the commits on the feature branch need to be rebased on-top of the revised master branch.

It is unfortunate that we need to rebase so many files with the repository cleanup! Hopefully this will remain a mechanical update.

@cekees this PR already removes a lot of code by avoiding the Cython signature duplication. As we did not want to change everything at once, we figured moving to xtensor structures would be a good intermediary step.

Regarding xframe, the choice to use xframe directly or another "dict of tensors" approach may depend on whether you want to use axes and named dimensions in proteus. We should definitely discuss this further.

[zhang-alvin]

I'm going to ahead and merge this, so we can move forward.

[JohanMabille]

Thanks @zhang-alvin ! Out of curiosity, why merging the master branch into the PR's branch (in genral) when there is no conflict instead of directly merging the PR into master? I think this (merging master into the PR) makes the history more complex and makes the rebasing of other PRs more difficult.

If there are conflicts, I think rebasing the PR is a better option since it keeps the history clean.

[zhang-alvin]

Thanks @zhang-alvin ! Out of curiosity, why merging the master branch into the PR's branch (in genral) when there is no conflict instead of directly merging the PR into master? I think this (merging master into the PR) makes the history more complex and makes the rebasing of other PRs more difficult.

If there are conflicts, I think rebasing the PR is a better option since it keeps the history clean.

I had just merged #1075 before merging this PR which introduced a conflict, which was resolved through the merge with master.

As for why not rebasing in this case, I felt like it would be another unnecessary detour to get this merged: you'd need to be alerted of the conflict, then you'd rebase, then the tests run, then we can merge. With the time zone differences, it appears to be a lot of effort when the PR will just be squash merged anyway. This probably merits a proper discussion among the developers.