Switch Carpet to new bboxset class implementation

Create issue
Issue #1729 closed
Erik Schnetter created an issue

Carpet has a new bboxset class implementation that scales to much larger number of MPI processes. I suggest to make the new implementation the default.

This is currently disabled, and can be enabled by adding the two flags "-DCARPET_ENABLE_BBOXSET2 -DCARPET_USE_BBOXSET2" to CPPFLAGS.


Comments (12)

  1. Ian Hinder
    • removed comment

    I seem to remember that this new class required C++11. Is that the case? bboxset2.hh seems to have some workarounds in case lambdas are not available, so maybe this is no longer the case?

    I have glanced at the code, but do not have time to do a detailed code review, as there is a lot of code there. I assume that if it passes the current tests, it is appropriate to make this the default at this point in the release cycle, and if there are problems, that change can be reverted before the next release. If you feel this level of review is appropriate, please go ahead.

    One comment: this is the sort of code for which it is very easy to write unit tests. Assuming they don't already exist, where should they go? I have some local additions to the old bboxset code, and wanted to write tests for them, but wasn't sure of the best way of organising that.

  2. Erik Schnetter reporter
    • removed comment

    Yes, this requires C++11. There are very few systems where C++11 is not available, and there one needs to use a -DCARPET_DISABLE_BBOXSET2 to fall back to the previous implementation (hello, Zwicky!).

    Yes, there are unit tests. They are in the thorn Carpet/TestBboxSet2.

  3. Ian Hinder
    • removed comment

    What sort of error message would people get at the moment if C++11 is not available, and they try to build the ET on a machine without the CARPET_DISABLE_BBOXSET2 macro defined? Should C++11 be detected automatically duing autoconf, and Carpet use this to decide whether the new code is used? Or should the check be more specific, for the features which are actually used?

    I am thinking of users trying to set up the ET on a new machine which does not have C++11 (are these sufficiently rare that we can assume users won't encounter them?) and getting an error message which does not clearly explain that they need to set a Carpet macro due to having an old compiler.

    Thanks for the unit tests; I actually looked for such a thorn, but for some reason didn't see it.

  4. Erik Schnetter reporter
    • removed comment

    The error message would likely (so I hope) complain about a C++11 feature not available, such as a range-based for loop or a lambda. If the compiler is really old, it may just be a syntax error. An autoconf macro would be nice.

  5. Roland Haas
    • removed comment

    Replying to [comment:3 eschnett]:

    Yes, this requires C++11. There are very few systems where C++11 is not available, and there one needs to use a -DCARPET_DISABLE_BBOXSET2 to fall back to the previous implementation (hello, Zwicky!). Please have a look at simfactory2's current master and the zwicky-intel14 set of files in there.

  6. Frank Löffler
    • removed comment

    Replying to [comment:6 hinder]:

    According to http://www.stroustrup.com/C++11FAQ.html#11, we could test the __cplusplus macro. See also http://stackoverflow.com/questions/5047971/how-do-i-check-for-c11-support.

    #if __cplusplus <= 199711L #error This library needs at least a C++11 compliant compiler #endif

    Maybe we could try this?

    If such a way to test for C++11 is available, then we should not output an error in that case, but define CARPET_DISABLE_BBOXSET2 instead (in addition to a warning maybe), shouldn't we?

  7. Ian Hinder
    • removed comment

    I wasn't suggesting that code fragment literally; I was using it as a way to illustrate how C++11 can be checked for. On the other hand, is it confusing to have the version of the code used depend on the version of the compiler? There should probably at least be a warning at compile time such as "C++11 not found; falling back to old bboxset implementation", so that it is clear that the preferred version is the C++11 version, and using the old version is not recommended in some sense.

  8. Ian Hinder
    • removed comment

    [Sorry Frank, I didn't read your comment carefully enough, and missed that you had already mentioned a warning.]

    To push this ticket forward, let me make a concrete suggestion:

    If the user sets the CARPET_DISABLE_BBOXSET2 macro or `if __cplusplus <= 199711L`, then fall back to the old bboxset implementation and emit a compile-time warning that the old implementation is being used because C++11 is not available.

    This should mean that people don't have to explicitly configure for this feature on their machine. Then, if the tests are OK, as they apparently are, then I am happy for this to be made the default at this point in the release cycle.

  9. Erik Schnetter reporter
    • changed status to resolved
    • removed comment

    I implemented this not by checking the reported standard version, but by adding autoconf macros for the required features. I also added a build-time warning if Carpet disables the new bboxset class.

  10. Log in to comment