Update to have the 'p' array written to the same file as 'q' and 'aux'

[weslowrie]

I was playing around with the option to have derived variables written out as described here: http://www.clawpack.org/pyclaw/classes.html#outputting-derived-quantities and noticed that it has two separate calls to the write function, which is different to how the 'aux' array is treated. I decided to combine it all into one call, and make it such that when using the 'compute_p' option, it would add a new dataset to the hdf5 file, or in the ascii case create a file with the name 'fort.pxxxx' to make it consistent with writing out the aux array.

This seems cleaner, but maybe there are some other factors I missed?

I didn't look into 'binary.py' or 'netcdf.py' or into the 'petclaw' directory. Also at first look, it does not look like the ability to read 'p' variables would be necessary?

[ketch]

This is part of a number of IO issues that we have never fully sorted out.

I think the current thinking is that writing q and optionally aux is what you need in order to restart the simulation from that point. Thus these files form a checkpoint. The p values, on the other hand, are purely for post-processing purposes. Thus one might want to output p much more frequently than q, or even turn off q altogether. That is not possible at present but would be easy to add under the current code. It has been important in the past to have p in a separate file also because of file sizes and the cost of transferring huge files from a supercomputer or cluster to another file system.

So I appreciate the cleanliness of the code (very much!), but I think this change runs contrary to what we want, long-term.

[weslowrie]

@ketch, thanks for the feedback, and that makes sense. I actually frequently have the issue of transferring these sometimes huge files over a network.

With that said, having a single call to the output routine with flags turning on/off output might still be useful? But for the output at different times/frequencies, it might make sense to keep the calls separate?

[ketch]

With that said, having a single call to the output routine with flags turning on/off output might still be useful? But for the output at different times/frequencies, it might make sense to keep the calls separate?

I'm not sure I understand -- seting controller.output_format = None does turn off all output.

This part of the code could definitely use some love. If you want to propose your vision for how it should work, that would be wonderful.

[weslowrie]

I'm not sure I understand -- seting controller.output_format = None does turn off all output.

I was thinking more along the lines of turning on/off each component of the output (q, aux, and p) even if they are going to be written to different files. The code currently has this capability, but makes two calls to the output routine.

This part of the code could definitely use some love. If you want to propose your vision for how it should work, that would be wonderful.

I'll think some more about possible ways to modify this part of the code.

[ahmadia]

I was thinking more along the lines of turning on/off each component of the output (q, aux, and p) even if they are going to be written to different files. The code currently has this capability, but makes two calls to the output routine.

This makes sense to me. @weslowrie - will you be at SIAM this week?

[weslowrie]

@ahmadia I was really hoping to make it to SIAM this week, but I couldn't make it work. Maybe the next similar conference.

[mandli]

What's the status on this PR?

[weslowrie]

I have abandoned this idea, as it probably does not make sense to combine the output files for primary variables and the derived variables. Maybe these routines can still be improved/optimized?

[ketch]

I agree -- we'd like to move to a model where we have checkpoint files (infrequent) and separately output files with whatever is needed for post-processing (more frequent). General improvements to this part of the code would certainly be welcome.

Based on Wes' comments, I'm going to close this PR for now.