Skip dtype_out_time options that cause errors?

[spencerkclark]

Short term issue

@chuaxr has a use case for computed variables that do not have a time dimension (i.e. their def_time attribute is False). She also has a use case for variables that do. It would be nice if she could submit calculations in the main script for both of these variables at the same time, with specifications to compute no time reduction (e.g. dtype_out_time=None) and specifications to compute a time reduction (e.g. dtype_out_time='av'), without having things halted by an error. More explicitly say one has two variables named without_time_dim and with_time_dim, and wants to compute them for a single Run and specifies dtype_out_time=['av', None]; the main script would create two Calc objects:

The first would seek to compute:

• without_time_dim with a time average
• without_time_dim with no time reduction

The second would seek to compute:

• with_time_dim with a time average
• with_time_dim with no time reduction

The one in bold currently raises an error, and crashes the set of computations. We ignore errors on Calc objects themselves, so the calculations for the with_time_dim variable would be fine, but since without_time_dim with a time average would crash, aospy would not get to computing without_time_dim with no time reduction. For consistency and convenience, it seems like it could be a good idea report, yet ignore the exception caused by attempting to compute without_time_dim with a time average (rather than error) in a similar manner to what is done for Calc objects:

aospy/aospy/automate.py

Lines 244 to 256 in ce7c784

	def _compute_or_skip_on_error(calc, compute_kwargs):
	"""Execute the Calc, catching and logging exceptions, but don't re-raise.
	Prevents one failed calculation from stopping a larger requested set
	of calculations.
	"""
	try:
	return calc.compute(**compute_kwargs)
	except Exception as e:
	msg = ("Skipping aospy calculation `{0}` due to error with the "
	"following traceback: \n{1}")
	logging.warn(msg.format(calc, traceback.format_exc()))
	return None

Longer term thoughts

This is a very specific issue caused by the current main-script / Calc pipeline approach, which might have a relatively simple fix, but it could also serve as motivation to explore ways to use aospy outside of this strict paradigm (e.g. it does not always make sense to compute the Cartesian product of all variables and options; right now I address this by creating separate main scripts, but I feel like there could be a better way to do this).

[spencerahill]

The one in bold currently raises an error, and crashes the set of computations

Where is this error occurring? I'm sure we can catch it and skip rather than let it crash. I agree, there's no reason why we should let this crash the whole CalcSuite whereas other errors get caught and those Calcs skipped.

it does not always make sense to compute the Cartesian product of all variables and options

I agree with this but haven't given it much thought beyond that. If you have some ideas, a new Issue about this would be great!

[spencerkclark]

The error occurs here:

https://github.com/spencerahill/aospy/blob/develop/aospy/calc.py#L483-L497

The issue is that variables that are not time-defined do not have a 'year' dimension, so xarr.mean(internal_names.YEAR_STR) raises a ValueError. I think we probably would want to handle the exception in here:

https://github.com/spencerahill/aospy/blob/develop/aospy/calc.py#L531-L550

when we create the reduced dictionary (basically if _time_reduce raises a ValueError we should just not include an entry in the reduced dictionary).

If you have some ideas, a new Issue about this would be great!

Sounds good; when I get a chance I'll try and play around and see how easy/hard this might be to do already.

[spencerahill]

Cool, that all sounds good to me.

[spencerahill]

Labeling this "low-hanging fruit" with respect to the short-term fix. The bigger picture concerns are obviously more involved.

[spencerahill]

In talking to @micahkim23 offline about his initial stab at this in #242, it occurred to us that it makes more sense to catch this earlier on, namely before the Calc object is even generated by CalcSuite.

Consider the case where a Calc is generated in which none of the reductions are valid, e.g. all of them are time-defined and the variable is not. With the logic previously proposed, we would still generate the Calc, load the data, perform the calculation, and then attempt to apply the reduction, which would finally lead to an error that we would catch and then move on. This seems like a waste.

Instead, what I'm proposing is for us to catch this within CalcSuite, such that invalid (Var, reduction) combinations get skipped over entirely. Here's a sketch of how this could work, through revision of CalcSuite.create_calcs (note this is 100% untested; logic may not be quite right):

_time_defined_reducs = ['av', 'std', 'reg.av', 'reg.std']

def create_calcs(self):
    """Generate a Calc object for each requested parameter combination."""
    specs_in = self._combine_core_aux_specs()
    specs_out = []

    calcs = []
    for spec in specs_in:
        reducs_in = spec['dtype_out_time']
        reducs_out = []
        if not specs['variable'].def_time:
            for reduc in reducs_in:
                if reduc not in self._time_defined_reducs:
                    reducs_out += reduc
                else:
                    logging.info('skipping this because blah blah')
        if reducs_out:
            spec['dtype_out_time'] = reducs_out
            specs_out.append(spec)
        else:
            logging.info('no valid reductions blah blah')

    return [Calc(CalcInterface(**sp)) for sp in specs_out]

So all of the Calc objects generated are guaranteed to have valid reductions.

There are a couple downsides, however:

1. This doesn't help if the Calc objects are generated by some other means than via CalcSuite. At least so far, that's not an important use case, but still.
2. This adds another place where we rely on parsing reductions as strings, when really reductions should be a proper class, c.f. Adding custom reduction methods #208 (comment). If we had a Reduction class with a def_time attribute, we could replace the if reduc not in _time_defined_reducs: line with if not reduc.def_time:

Does anybody (esp. @spencerkclark) have any thoughts? We need to make a decision here one way or the other before @micahkim23 can proceed with #242. I would be in favor of going this route, although I'm deep enough into it that I may be missing something important.

[spencerkclark]

@spencerahill @micahkim23 many thanks for bringing this up; it was something that had not occurred to me initially. I agree that we should do everything we can to prevent loading data when we don't need to (since that can be fairly time consuming). I'll need to think over this a little more, but I just have one quick question for now.

1. This doesn't help if the Calc objects are generated by some other means than via CalcSuite. At least so far, that's not an important use case, but still.

I could be missing something, but is there some way this could be handled directly in the Calc constructor rather than in automate.py? We should have all the same information at our fingertips there (e.g. the Var object and the time reductions requested) that we need to use to assess whether a calculation makes sense. This would solve the issue of Calc objects generated outside of automate.py.

[spencerahill]

@spencerkclark yes we could definitely do that; meant to mention that.

Doing it within CalcSuite has the conceptual/aesthetic benefit that all of the Calc objects created are then guaranteed to have a valid combination of parameters. But from a more practical perspective, there is little cost to creating a Calc object; it's really in subsequent data loading and computation that we take a big hit.

We could do it in both. The logic in CalcSuite would remain the same as above; within Calc it would likely be similar (less the looping over different specs) but I suspect will entail more design decisions.

Extending that line of thought: the more I'm thinking about it, the more I like the idea that we would have built-in guards like this, ultimately for all of our classes. If a user inadvertently attempts to generate an object or submit a calculation that is inherently invalid, we should step in. #84 and #136 are in the same vein.

[spencerahill]

Forgot to mention, we should make a decision on this soon if possible, as @micahkim23 is needing to wrap up his work for the quarter basically by the end of next week(end).

I am in favor of implementing the checks in both places, CalcSuite and Calc. For Calc, I agree that __init__ is the place to do this. The question becomes what do we do, keeping in mind that there will be cases where some, but not all, of the dtype_out_time options are valid; cases where they are all valid; and cases where none of them are valid.

What immediately comes to mind:

1. Check each dtype_out_time value for validity, flagging it as valid or not
2. Warn for each invalid value that it is invalid and will be skipped.
3. If all are invalid, then raise an error when Calc.compute gets called so as to prevent any data being loaded or anything else being done. (Or does data get loaded within __init__? If so we should raise there.)
4. If some are valid, proceed as normal, but skip invalid values when executing the calculation.

@spencerkclark let me know your take.

[spencerkclark]

We could do it in both. The logic in CalcSuite would remain the same as above; within Calc it would likely be similar (less the looping over different specs) but I suspect will entail more design decisions.

Could we get away with not modifying CalcSuite at all? I'm thinking adding a call to something like the following in Calc.__init__ would accomplish all that we need in this case:

_TIME_DEFINED_REDUCTIONS = ['av', 'std', 'reg.av', 'reg.std']

def _prune_invalid_time_reductions(self):
    valid_reductions = []
    if not self.var.def_time:
        for reduction in self.dtype_out_time:
            if reduction not in _TIME_DEFINED_REDUCTIONS:
                valid_reductions.append(reduction)
            else:
                logging.info('Skipping this time reduction, because ... ')
    else:
        valid_reductions = self.dtype_out_time
    self.dtype_out_time = valid_reductions

Calc.__init__ does not load data, so it should be fast to do this check. In addition, if an empty list is provided for self.dtype_out_time, I'm pretty sure nothing will be done (I think it would be OK if we did not raise an error here, though we still could under this setup if desired). Since CalcSuite.create_calcs just creates a list of Calc objects, all the logging messages written by the Calc constructor would appear appropriately.

Having a check in CalcSuite would prevent one from creating an "empty" Calc object in the rare event that no valid reduction operations exist, but I'm not sure if the cost of that is significant enough to merit duplicating the logic.

@spencerahill what do you think? I think really this is just a discussion about aesthetics at this point :)

[spencerkclark]

It just occurred to me that an argument could be made for the following instead:

• In Calc.__init__ raise an informative error if any invalid reduction in dtype_out_time is provided.
• In CalcSuite.create_calcs add logic to prevent invalid time reductions from being passed to Calc objects.

I think one could argue that CalcSuite is just one convenient way to create and execute a group of Calc objects. Skipping invalid calculations could be considered a "feature" of the CalcSuite path of execution. Having skipping invalid time reductions as the default behavior in Calc seems a bit unnatural to me (I feel like it's assuming too much). If one wanted to make Calc objects outside of CalcSuite, one would need to make sure that valid reductions were specified.

@spencerahill what do you think about that argument?

[spencerahill]

@spencerkclark, glad as usual to have gotten your feedback, as I think your last suggestion (drop invalid reductions in CalcSuite and raise in Calc if any are invalid) is the best one.

Having skipping invalid time reductions as the default behavior in Calc seems a bit unnatural to me (I feel like it's assuming too much).

I agree, too automagical.

@micahkim23, please proceed along these lines. Ping us when you're ready for a review.

Thanks both!