search

clawpack / visclaw

Clawpack visualization tools
http://www.clawpack.org
BSD 3-Clause "New" or "Revised" License
29 stars 48 forks source link

Replace `gettime` function with an `OutputController` from PyClaw #145

Closed mandli closed 9 years ago

mandli commented 9 years ago

This uses the proposed functionality in https://github.com/clawpack/pyclaw/pull/508.

mandli commented 9 years ago

It is dubious that we should be creating a new controller every time this function is called but I wanted to have a minimal number of code changes to try this out. The controller should probably be an attribute of the data object.

ketch commented 9 years ago

Yes, my preference again would be to just have a gettime function that does not belong to an object, at least until we flesh out in more detail what that object should be. But this looks fine if you prefer creating the object.

mandli commented 9 years ago

The reason why I put the gettime function as a member function of the class is that the state needed is stored in the class so I felt that it would be best to have the get_time function exist there. Otherwise we would have to pass in the path to the output and the format again (unless we finally implement a consistent way of determining the output). My view is that one of the reasons VisClaw has some issues with this is that it does not consistently store the state needed to perform IO.

mandli commented 9 years ago

I just pushed up a version that keeps the controller in the data object. Part of the problem that I mentioned is readily seen by the lines just above where I put the new object, lots of copies of things keeping the same data leading to problems later.

ketch commented 9 years ago

I spent more time thinking about the bigger picture here.

What is gettime trying to do? Find the time associated with a Solution; i.e. solution.t. It makes the assumption that the Solution is specified via a frame number, path and file format, and it avoids the need to read in the whole solution (i.e., q) just to get t.

What we really ought to do is avoid these assumptions and make it possible to get this information by just asking for solution.t whether the solution is in memory already or on disk. I see two options here:

  1. Assume that if we need solution.t then we'll also need solution.q at some point, so we can just get rid of gettime and refactor the code that uses it.
  2. Make it possible to initialize a Solution object via a frame number, path, and file format but without necessarily loading solution.q. The Solution could cache the path to where it is on disk, and attributes like t and q could be properties that get populated (by reading from disk) only when asked for.

@mandli What do you think?

mandli commented 9 years ago

I think it is probably important to peruse the second option as there are current use cases where the time of all the solutions is desired before the data associated with one solution is used. Perhaps this behavior could be modified but it seems like it would be better to only load data when asked (through properties for instance). My one hesitation is regarding whether the solution should retain the data required to do this. Does it belong there or should we be looking I to a factory type of approach to the design?