solution.Solution -- cannot pass `frame=` as kwarg

[rjleveque]

trafficstars

I was having problems reading in a single frame of a solution using solution.Solution and finally tracked it down to the fact that although the first parameter of Solution is named frame you cannot call it using this as a kwarg, e.g. the two commands below should load the same frame but the second silently does nothing:

>>> from clawpack.pyclaw.solution import Solution

>>> framesoln = Solution(0, path='_output', file_format='ascii')
>>> len(framesoln.states)
18

>>> framesoln = Solution(frame=0, path='_output', file_format='ascii')
>>> len(framesoln.states)
0

The reason is that the code decides whether to read the solution based on this logic:

        if len(arg) == 1:
            # Load frame
            frame = arg[0]

Surely there's a better way to get the desired behavior but also allow specifying the key word?

And if this gets rewritten, would it be better to call this argument frameno for consistency with what we do in visclaw and other places, or is frame too heavily used elsewhere in pyclaw?

[ketch]

Yeah, the fact that the initialization does nothing if there are zero or 3+ arguments is very bad.

Looking through the code, frame is used quite heavily; there are also instances of both frameno and frame_number....

[ketch]

Edit: and frame_num. To be fair, I think this is the result of many different people editing the code.

[rjleveque]

It's no big deal to have different names for frameno I think, but it would be nice to make it more robust to the manner in which it is called.

[mandli]

Looking at the current code I am not sure I see a way out of this dilemma without breaking something. Some thoughts though:

• Best solution would probably be to change all arguments to key word arguments with frame being first, which would then maintain the 1 argument version.
• Maybe we could code around the 2 argument problem by checking the type of frame and treating it as a domain or patch although I am not entirely sure I remember.
• We would want to maintain a specific set of key word arguments that would be explicitly handled in the __init__ routine and pass the rest to the call to read as is done now.

[ketch]

I think we can do something reasonable in a backward-compatible way:

• Add a case for len(args)==0, where we check which keywords have been specified
• Also add an exception at the end so initialization never silently fails

If we are willing to make incompatible changes in the future, this could be revamped along the lines that Kyle suggests.

[mandli]

Agreed. Better to not break things.

[ketch]

I've implemented what I suggested in #636 . Could you both take a look?

[rjleveque]

That fixes the problem I was having at least. I haven't tested it more extensively.

I'm not sure what the recommended approach is more generally for cases like this where you want to be able to initialize in various different ways with the same __init__ function. We face something similar e.g. with topotools.Topography where you could pass in both a path to a topofile and a topo_func that computes the topography instead. The path doesn't actually get used unless you also calls the read function. It's potentially confusing.

[mandli]

I think keyword arguments are probably the right thing to use for this type of functionality and has the benefit of being less confusing. The one problem is if someone uses incompatible keywords but then you could check for that.