Modify

Opened 3 years ago

Last modified 2 years ago

#1853 confirmed defect

diagonal output in CarpetIOHDF5 disabled

Reported by: Roland Haas Owned by: Erik Schnetter
Priority: minor Milestone:
Component: Carpet Version: development version
Keywords: Cc:

Description

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.

Attachments (0)

Change History (15)

comment:1 Changed 3 years ago by Ian Hinder

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

comment:2 Changed 3 years ago by Roland Haas

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.

comment:3 Changed 3 years ago by Ian Hinder

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.

comment:4 Changed 2 years ago by Roland Haas

Milestone: ET_2016_05

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

comment:5 Changed 2 years ago by Frank Löffler

What was the reason for disabling it in the first place?

comment:6 Changed 2 years ago by Roland Haas

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.

comment:7 Changed 2 years ago by Frank Löffler

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.

comment:8 Changed 2 years ago by Ian Hinder

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.

comment:9 Changed 2 years ago by Erik Schnetter

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.

comment:10 in reply to:  9 Changed 2 years ago by Frank Löffler

Replying to 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?

comment:11 Changed 2 years ago by Erik Schnetter

See Roland's comment above.

comment:12 Changed 2 years ago by Roland Haas

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.

comment:13 Changed 2 years ago by Frank Löffler

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)

comment:14 Changed 2 years ago by Ian Hinder

Status: newconfirmed

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).

comment:15 Changed 2 years ago by Frank Löffler

Milestone: ET_2016_05

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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as confirmed The owner will remain Erik Schnetter.
Next status will be 'review'.
as The resolution will be set.
to The owner will be changed from Erik Schnetter to the specified user.
The owner will be changed from Erik Schnetter to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.