diagonal output in CarpetIOHDF5 disabled

Issue #1853 resolved
Roland Haas created an issue

Commit 37f5f2e "CarpetIOHDF5: Optimize lower-dimensional output -- only transfer necessary data to the I/O process" disables diagonal output, yet the parameters for it are still present.

Lack of diagonal output in hdf5 files is most likely acceptable, but the fact should be at least documented.

Keyword:

Comments (20)

  1. Ian Hinder
    • removed comment

    I have used diagonal output in HDF5 files recently. Why should it be disabled? Was it disabled accidentally?

  2. Ian Hinder
    • removed comment

    I meant that I had used it recently, and hence that the feature is useful, not that the code was recent. Here, recent meant within the last 6 months. I am not claiming that it is working; I am saying that it should probably be put back, unless there is a good reason not to.

  3. Roland Haas reporter

    This must be addressed before the release otherwise diagonal hdf5 output will not work anymore.

  4. Roland Haas reporter
    • removed comment

    It was collateral to a change that significantly reduces memory requirements for 1d,2d output in HDF5 (and likely would also in ASCII output from Carpet). The previous implementation sends the full 3d data to a single process which then extracts the lower-d data and writes it to disk. For sufficiently large grids this is very slow (significant fraction of runtime) and runs out of memory. The breaking patch fixes the issue be doing the dimensional reduction before sending data. For diagonal data the reduction is not as straightforward and was not immediately implemented.

  5. Frank Löffler
    • removed comment

    With no efficient implementation of diagonal output on the horizon we have to decide: - Would it be possible /feasible to have it work, inefficiently only for diagonal output? If no: - What is more important: still providing diagonal output, or having the regular output efficiently?

    I lean towards reverting the patch for the release, but only if there is an anticipated push to get this fixed soon after. Otherwise this is a change that gets pushed from release to release and that shouldn't happen. If we know that nobody is interested in implementing something that works for all we better decide now.

  6. Ian Hinder
    • removed comment

    I am unhappy about removing a feature like this. Diagonal HDF5 output is useful in some situations (I have used it for black hole lattices, in which the diagonal is a special line on which we are very interested in the data). It is not very well supported; most of the metadata is missing unfortunately. I am in favour of reverting the patch and waiting for it to be implemented in a way that allows diagonal output to be maintained.

  7. Erik Schnetter
    • removed comment

    The choices here are either "no diagonal output" or "runs out of memory" (see above). I would not revert a change that corrected a problem just before a release, as this might make the release unusable for large simulations.

  8. Frank Löffler
    • removed comment

    Replying to [comment:9 eschnett]:

    The choices here are either "no diagonal output" or "runs out of memory" (see above).

    In all simulation sizes? I regularly do simulations that use lower-dimensional hdf5 output and haven't seen a memory problem there. Maybe my grid sizes are not "large enough" to trigger this, so then: which sizes are?

  9. Roland Haas reporter
    • removed comment

    Just piping up: as far as I remember, the simulation that failed is one that had a single rather large refinement level (only data from one reflevel is copied), as often happens during supernova simulations. The memory reduction in that case is quite large of course since one goes from 3d data to 2d (or even 1d) data.

    Having said that, I have to admit that I am actually more in favor of reverting the change for the release version. I favor this option because current master fails to deliver promised functionality for all users while the version before the patch failed to promise functionality only for some users that know why this happens and how to avoid it (and the users to whom this originally happened are running off master anyway and not off the release branch). So my favored solution (given that I do not see a "proper" handling of diagonal output to be implemented, tested and trusted very quickly), would be to remove the patch the reduces memory consumption at the cost of disabling diagonal output for the release branch only.

    My understanding of "development happens on master" and its relationship with release branches is that master may always break anything but that release branches may only break things after a one release announcement that this will happen.

  10. Frank Löffler
    • removed comment

    Let's discuss this in today's call. I favor Roland's proposal of reverting this for the release, in the hope of finding a full, fast implementation for the next. (and also not reverting this for master)

  11. Ian Hinder
    • changed status to open
    • removed comment

    In the call, we decided to revert this in master before the release. It was suggested that a working (though inelegant) solution may not be hard to implement, and this could happen after the release. I was not involved in this code, and I couldn't find a clear set of commits to revert. It seemed that the change was spread across more than one commit, and that one of the commits also changed other things. I think this would be better handled by someone who is more familiar with the changes that were made (i.e. Erik or Roland).

  12. Frank Löffler
    • removed milestone
    • removed comment

    A revert (only reverting part of the commit that was relevant to diagonal output) was pushed to master. I'll leave the ticket open to get "proper" lower-dimentional output implemented in a branch, tested, and merged back to master.

  13. Log in to comment