On a CannotCalculate exception, xtype processing should stop and an appropriate value returned

[bellrichm]

No description provided.

[bellrichm]

Something needs to be done for the get_aggregate function at line 128. I think we want something like this added to the try/except

        except (weewx.CannotCalculate):
            # Look up the unit type and group of the observation:
            t, g = weewx.units.getStandardUnitType(db_manager.std_unit_system, obs_type,
                                                   aggregate_type)
            # Return as a value tuple
            return weewx.units.ValueTuple(None, t, g)

[tkeffer]

Got to be careful about separating implementation from policy. The implementation detects that a type cannot be calculated, but it's policy what to do about it, such as setting the value to None.

As an example has_data() doesn't need or want a None value. It needs a type bool.

That's the theory, although I don't think xtypes is entirely consistent about this.

[bellrichm]

That probably explains why I am unsure… So let’s step back.
I have an xtype that is not backed by a database field (calculated on the fly). Seasons skin is making a call for mintime aggregation. My xtype doesn’t ‘support’ this aggregation, so it doesn’t matter if my xtype is prepended or appended, XtypeTable is going to attempt to calculate it. At line 942 the call to get_scalar eventually results in a CannotCalculate exception from my xtype (because the necessary inputs are missing). If this exception is not handled, it bubbles all the way up.
So, it feels right in this case (maybe not all?) that the aggregation is None… But I have to admit these xtypes are giving me a headache :)

Maybe we should just deal with has_data in this pull request and move this discussion to WeeWX development group? Your call.

[tkeffer]

Gives me a headache too, and I invented it! I think the idea is sound, but details of the details in the implementation can be challenging.

Sounds like Seasons is trying to calculate something like $month.mytype.mintime. If that's the case, the other place where a CannotCalculate exception could be handled is in tags.py, line 500. This is a little higher in the call stack. Would look something like this:

        try:
            # If we cannot perform the aggregation, we will get an UnknownType or
            # UnknownAggregation error. Be prepared to catch it.
            result = weewx.xtypes.get_aggregate(self.obs_type, self.timespan,
                                                self.aggregate_type,
                                                db_manager, **self.option_dict)
        except (weewx.CannotCalculate):
            # Look up the unit type and group of the observation:
            t, g = weewx.units.getStandardUnitType(db_manager.std_unit_system, self.obs_type,
                                                   self.aggregate_type)
            # Return as a value tuple
            result = weewx.units.ValueTuple(None, t, g)
        except (weewx.UnknownType, weewx.UnknownAggregation):
            # Signal Cheetah that we don't know how to do this by raising an AttributeError.
            raise AttributeError(self.obs_type)

But, I'm not sure that it's better than your solution.

[bellrichm]

My ‘argument’ for where I proposed is consistency. If we put the handling in tags.py we should probably move the has_data like I did in commit.
At the time I tried this I did not understand the nuance of CannotCalculate vs other exceptions, so I withdrew it.
My final point is, handling it in the xtypes stack means any/all callers can expect a value to be returned and not have to handle the exception.
Ultimately, it seems 6 of one 1/2 dozen of another… Let me know how you want to proceed.

[tkeffer]

I'm trying to come up with a regression test for your PR, but I'm having trouble. I cannot get the line

            vt = xtype.get_aggregate(obs_type, timespan, 'not_null', db_manager)

to raise weewx.CannotCalculate. The reason is that if the type is an xtype, not_null can always be calculated.

What are you seeing to trigger your addition?

[tkeffer]

Never mind. I figured out a test case.

[bellrichm]

Here is what I think happened…
Seasons skin checks if myxtype has_data within the last 30 days (line 43). It then tries to get maxtime (line 110) for day, week, etc. Unfortunately the sensor that supplies the data for myxtype has been offline for a while and therefore maxtime cannot be calculated for day or week.
Unfortunately my data has changed since I first saw this. Let me know if you want me to try to recreate it.

[tkeffer]

Take a look at the bellrichm-CannotCalculateFinal branch and see if that works for your extension.

[bellrichm]

For has_data yes. But what would you expect to happen/test for something like this.

    def test_get_aggregate_none(self):
        """Test get_aggregate() with a type that cannot be calculated"""
        with weewx.manager.open_manager_with_config(self.config_dict, 'wx_binding') as db_manager:
            with self.assertRaises(weewx.CannotCalculate) as error:
                result = weewx.xtypes.get_aggregate('fooTemp', month_timespan, 'mintime', db_manager)
                print(result)
            print(error.exception)

I would say None should be returned, but as you pointed out the exception could be handled in tags.py.

I am all for merging this PR and if required, having a separate for get_aggregate. I only started the conversation here in case it would have any influence over the implementation for has_data.

[bellrichm]

FWIW, a call to weewx.xtypes.get_aggregate with an observation in the db, mintime aggregate, and all nulls in the db returns ‘None’. So if my xtype was in the db it would return ‘None’.

[tkeffer]

I tend to agree. If one is calculating an aggregate of a synthetic type over an interval, and one or more of its ingredients are missing in that interval, then those values should be ignored. The rest will still be valid.

Currently, we set the results of the aggregation to None. This change would return a value, albeit with not as many constituent ingredients.

[bellrichm]

I think we are getting close...
Currently if you do something like $day.myxtype.mintime and if the necessary ingredient is null in any archive_interval for that day, a CannotCalculate is raised and is not handled.
If you look at XTypeTable.get_series line 902 the exception from get_scalar is caught and treated as though a None value was returned.
I propose that when XTypeTable.get_aggregate calls get_scalar at line 942, the same logic should be applied. The CannotCalculate exception is caught and it is treated as though a value of None is returned. This would allow aggregration of any values that can be calculated.

[tkeffer]

That's exactly where I was headed.

Merged into master. I think we're done, but give it a try.

Thanks, Rich. I appreciate the effort it took to dive into the details of the XTypes system!

[bellrichm]

Yup, I think we are done. Thanks for bearing with me.
I’m still dizzy, but off now to fix my implementations of get_series and get_aggregate.
Thanks again for WeeWX!