GeoClawData.read: manning_break with single value is read as float instead of list

[tovogt]

When using the function GeoClawData.read on a "geoclaw.data" file that has been previously written using the function GeoClawData.write, the manning_break attribute might be read as a float instead of a list if it only contains a single value. This is because the function clawpack.clawutil.data.ClawData.read can only recognize lists correctly if they contain more than a single value.

I would really love to have an implementation of GeoClawData.read that restores the original manning_break attribute. In my applications, I usually have manning_break = [0.0]. When reading in the data, modifying it and attempting to write again, an error is raised because GeoClawData.write expects that manning_break is a list-type attribute.

Note that other attributes might be affected by the same issue and it might be worth going through all subclasses of ClawData and making sure that list-type attributes are read as actual list types.

[mandli]

This is duplicated functionality as the FrictionData object handles this type of friction along with file based friction specifications (although this is not implemented). Unless there is a reason to keep this we should deprecate its use in favor of FrictionData.

[tovogt]

Thanks for the response! Is FrictionData documented somewhere? I understand from the Ike example that the format is a bit different from the manning_break attribute. While the latter "should be a monotonically increasing list of length N-1" (docs), the depths attribute seems to be a monotonically decreasing list of length N+1 (?).

[mandli]

FrictionData was created to handle any kind of friction coefficient data but never was fully implemented. The documentation is an oversight as it is documented but was never plugged into the general data page (we probably should fix that as there are some other data objects that are not included). We should probably figure out how to combine these functionalities. @rjleveque do you have thoughts on this?

[rjleveque]

Regarding using GeoClawData.read on manning_break, the best way to handle reading this as a list might be to define a read function for this class that calls clawutil.data.ClawData.read and then adjusts this parameter to be a list if appropriate. Right now it is a subclass that just uses clawutil.data.ClawData.read directly.

Regarding FrictionData, I have never used this myself but it does seem from the Ike example that in a FrictionData.friction_regions list there is a list of values that is decreasing rather than increasing and I wonder if increasing would be both more intuitive and more backward compatible.

Also I think what is called depths in FrictionData really refers to manning_breaks, i.e. the topography values B where the code switches from one Manning value to another, not fluid depths. E.g the Ike example includes these lines:

    # La-Tex Shelf
    data.friction_regions.append([(-98, 25.25), (-90, 30),
                                  [np.infty, -10.0, -200.0, -np.infty],
                                  [0.030, 0.012, 0.022]])

and the last two lists are really manning_breaks and manning_coefficients.

In the original implementation there is also a manning_depth that determines whether or not to apply any friction, based on the current depth of the water. This seems to be missing from the new FrictionData implementation? It makes sense to determine whether to apply friction based on the present depth (in sufficiently deep water friction has little impact) but that the Manning coefficient then used depends on the initial topography (e.g. if there is vegetation onshore and smooth sand offshore, since these topographic features don't change with the fluid depth during inundation). Both things are needed in src2.f90.

Finally, the FrictionData class includes a write function but no read function, I believe, so it does not yet seem to provide what @tovogt requires.

[mandli]

Thanks @rjleveque, I am not entirely sure why I had started from scratch with the functionality in FrictionData although part of the additional functionality does include regions for the definitions but otherwise is the same. The file specified friction has not yet been implemented but the thought there was that you could specify both using the regions again. #557 adds the read function but this could be easily adapted for the manning_breaks code. The last big difference is that the determination of the friction values is done as another aux array in preparation for more complex friction fields and so also provides a way to set this via friction_index that allows for moving the location in the aux array that these values are stored. I wonder that in order to not put a burden on memory if this should use what is done in manning_breaks when friction_index is set to -1 or something (I have on occasion wanted to have the field recorded to look at how the AMR is impacting the determination of the friction).

I guess what we should really decide is how to standardize the code and make sure all the options are there that should be. I am not entirely sure how best to do that but I agree with some of your points regarding what things should be made similar.