Add model ready-ness checking

[jrleeman]

Checks to make sure things are set before the model tries to run. This may be a naive way to do this, maybe I should raise a more specific ValueError exception for each case in the readyCheck function? Addresses #6

[dopplershift]

I have some code I've been working on lately that needs to do validation, and I haven't come up with a good answer there. Options I can think of:

• Just return True or False (like you implemented) Leaves the user guessing what's wrong
• Raise exceptions. Could also do this conditionally, based on a parameter, but that would make the code uglier
• Use the logging module to print warnings.

It's probably good to step back and consider what the use case is for this function, and, as your own user, how you'd be using this function.

[jrleeman]

It does seem like choosing amongst evils. I like raising exceptions because we can specifically say which variable is missing. That could be done just replacing the returns, but it is ugly. logging would do the trick and then just let the program crash right? Without all parameters set the integration will die. Maybe that is an okay solution, but still leaves a big function full of if and elif right?

[dopplershift]

You could probably do something like:

def maybe_raise(msg, raise, ret):
    if raise:
        raise ValueError(msg)
    return ret

def readyCheck(self, raise=False):
    if self.a == None:
        return maybe_raise('Need to set a', raise, False)
    return True

I'm not sure about names and parameter order, but you get the idea. The one question I have is: is readyCheck something the user has the option of checking, or are you doing it at the start of running the simulation?

[jrleeman]

I was planning to have the solve() method call it automatically do the user cannot run an incomplete model.

[dopplershift]

Then I'd say exceptions are the way to go. In that case, I wouldn't even bother with the conditional, just raise exceptions. You could use your own exception rather than ValueError:

class IncompleteModelError(Exception):
    pass

[jrleeman]

Ok. I implemented something along those lines. In doing so I had to add an __init__ call in the Model class to be sure that things like time and loadpoint_velocity were defined as None.

Doing this brought my attention to two things that should be addressed. 1) In the StateRelation objects __init__ method we have what looks to be a spurious relation argument.

class StateRelation(object):
    """
    Abstract state relation object that contains the generally used atributes
    in state relations (b,Dc).
    """
    def __init__(self, relation):
        self.b = None
        self.Dc = None
        self.state = None

2) How are b and Dc defined when we make an instance of DeiterichState for example when we don't call the __init__ method of StateRelation from them? If I comment out setting b in an example, we raise the appropriate error.

[dopplershift]

1) Looks like it.

2) If you haven't defined an __init__ in the subclass (like DeiterichState), the one from the parent class is called.

Overall, I'd say this looks good.

[jrleeman]

If we remove the extra relation argument, then we must change the examples and tests to not setup a state variable like state1 = rsf.DieterichState(model), but state1 = rsf.DieterichState()I see no problem with that.

[dopplershift]

Ah, I missed that. I think it improves things removing it, though.

[jrleeman]

Agreed. I'll go ahead and merge this. I can't think of a reason that we would need it in there.