Cygnus

[bwvdnbro]

Attempt to make a clean pull request for #54

I manually rebased the master and cygnus branches in the mapetkova:CMacIonize fork against the upstream master and resolved all conflicts.

@mapetkova: could you please check that this contains all the changes you made? I know some of the specific tweaks to CMakeLists.txt necessary to compile on cygnus are missing, but apart from that all the code changes you made should be there. Once you approve this pull request, I will make sure it gets merged into the development branch.

[bwvdnbro]

Looks like the automated unit tests fail. Reasons: the Clang compiler does not like the presence of an unused _log variable in the AsciiFilePhotonSourceDistribution class. GCC is fine with this, but fails the testSPHArrayInterface unit test because the expected values for that test assume the old implementation. I will fix these issues once the other changes get approved.

[bwvdnbro]

@mapetkova Sorry for requesting the review again (which I think happened automatically when I pushed another commit).

I had to fix a compilation error found when running the automated unit tests with the Clang compiler, and I also applied code formatting using clang-format to the new code (so that the code is in line with the project coding standard).

More importantly, I had to change the reference values in testSPHArrayInterface, the automated unit test that checks the code in SPHArrayInterface. Would you expect a difference of ~50% in total hydrogen mass between the old mapping (just the closest particle) and your new mapping algorithm? If you have time, it would be really useful if you could have a look at the code in test/testSPHArrayInterface.cpp and tell me if that looks okay to you. This test is meant to quickly flag unexpected and unintentional problems with the mapping algorithm caused by changes in other parts of the code. It makes total sense that it would flag if you make intentional changes to the mapping algorithm, as in this case. So if you are happy with the new values, it would be perfectly okay to just overwrite the old ones. But I would like to be sure.
Right now, there is no test for the inverse mapping algorithm. The old test only worked for the naive inverse mapping algorithm, and I don't really know how to set up a good test for your inverse mapping. Any thoughts on this are welcome. I have disabled this test for now; no need to wait for this to merge the pull request.

[mapetkova]

@bwvdnbro I looked at the changes and I got a rough idea of what the tests do. I'm not sure what the exact numerical values should be, but the main difference between the old mapping and my mapping is that mine should (more or less) conserve the total mass that's in the particles. The difference of the result between the old and the new density mapping should depend on the exact particle distribution, so I can't say if it would be within 50% (although it probably would be). If I were you I would set the reference value for the total hydrogen mass to be the total particle mass and I would accept values within, say, 10% of that (this is to account for particles which might be partially extended outside the simulation box).

[bwvdnbro]

Okay. I will rebase against development and merge.