Hey @valeriupredoi , this is one of the points that really confuses me. What happens if different cubes have different time offsets? Don't you want the offset with respect to a common reference time? I'm thinking something like

def _datetime_to_int_days(cube, overlap=False):
    """Return list of int(days) with respect to a common reference.

    Cubes may have different calendars. This function extracts the date
    information from the cube and re-constructs a default calendar using
    the datetime library.

    This doesn't work for daily or subdaily data, because different
    calendars may have different number of days in the year.

    The standard calendar used by datetime provides the to_ordinal()
    function which returns "int number of days since the 0001/01/01".
    """
    # Convert to a common time format
    years = [cell.point.year for cell in cube.coord('time').cells()]
    months = [cell.point.month for cell in cube.coord('time').cells()]
    if not 0 in np.diff(years):
        # yearly data
        standard_dates = [datetime(year, 7, 1) for year in years]
    elif not 0 in np.diff(months):
        # monthly data
        standard_dates = [datetime(year, month, 15) for year, month in zip(years, months)]
    else:
        raise ValueError("Multimodel only supports yearly or monthly data")

    return [dt.to_ordinal() for dt in standard_dates]

I can work this out further if you like?

yes that's exactly that, I use the number of days since the start of the data taken from units forcing a STANDARD_CALENDAR eg days since 1950-01-01, calendar=STANDARD_CALENDAR - this works fine if there is no variation in calendars since the units are all the same for all cubes after cmor.fix unified the units, but I suspect it's the cause of the shifts @LisaBock is seeing when calendars differ

btw isn't it toordinal()?

btw isn't it toordinal()?

Right you are 🍺

this works fine if there is no variation in calendars

So I'd rather just fix it, either to the ordinal or to 1950/01/01. Taking it from the cube seems unnecessary and error-prone.

yeah not the best code to understand indeed! but it does do a lot of tricksy stuff nonetheless. Can I suggest we finish up with this PR in terms of functionality for the yearly (normal) data and merge then we look into Lisa's issue, the more I look at it the more it looks to be way separate 🍺

I agree we should respect the scope of the PR, but let's not merge to hastily. I'm not so keen on the overlap argument for the yearly data. But removing it in favor of the above suggestion breaks several tests. Can I look into fixing that first?

that would be very much appreciated mate and will buy a 🍺

So I fixed most of it, but some tests still fail because test cube 2 has a daily time coordinate, and the code above raises an exception in that case. What to do with that?

let's not make the changes - your suggestion below saved the day, so let's keep the changes to a minimum and only commit what's necessary - let me take it from here 🍺

I think this can also be simplified:

    bounds = []
    for cube in cubes:
        span = _datetime_to_int_days(cube)
        start, stop = span[0], span[-1]
        bounds.append(range(start, stop+1))

But why even use the range function? Don't we just want the overlapping time points from span?

    spans = []
    for cube in cubes: 
        spans.append(_datetime_to_int_days(cube))    
    time_pts = reduce(np.intersect1d, spans)

mate can we please not start trying to fix something that's not broken? We need to understand the very specific problem that @LisaBock is having and to allow for the correct handling of yearly data - the second one works fine so let's just focus on the scope of this PR. I really don't want to start changing this MM right before the release 🍺

Sure, that's fair. It's just that I thought it might help to debug, for I'm still struggling to understand these parts of the code.