- removed comment
diagonal output in CarpetIOHDF5 disabled
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)
-
-
reporter - removed comment
There is a commit https://bitbucket.org/eschnett/carpet/commits/37f5f2e6ba973f1d8d5b1ddf194e844d876b7880 that puts an assert(0) into the branch of an if statement handling diagonal output (https://bitbucket.org/eschnett/carpet/commits/37f5f2e6ba973f1d8d5b1ddf194e844d876b7880#LCarpetIOHDF5/src/OutputSlice.ccT1460). You may want to check how "recent" your code is.
-
- 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.
-
reporter - changed milestone to ET_2016_05
- removed comment
This must be addressed before the release otherwise diagonal hdf5 output will not work anymore.
-
- removed comment
What was the reason for disabling it in the first place?
-
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.
-
- 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.
-
- 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.
-
- 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.
-
- 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?
-
- removed comment
See Roland's comment above.
-
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.
-
- 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)
-
- 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).
-
- 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.
-
reporter -
reporter Please review.
-
reporter Unless objected I will apply this after 2021-03-10.
-
reporter -
reporter - changed status to resolved
- edited description
- Log in to comment
I have used diagonal output in HDF5 files recently. Why should it be disabled? Was it disabled accidentally?