Additional fixes for PR #158

[tomvothecoder]

Description

This PR addresses the following issues (for PR #161):

• Incorrect time values being decoded in decode_time_units() for non-CF compliant time units
  • The fix is to use the time values as offsets to the reference date in the "units" attribute
• Calling open_mfdataset(decode_times=False) when datasets have the same numerically encoded time values, but differing non-CF compliant time units (e.g., "months since 2000-01-01", "months since 2001-01-01"), resulting in time values being dropped.
  • The fix is decoding time for each dataset individually in the preprocessing step before concatenating.
  • Performance is comparable between decoding each dataset before concatenating, and concatenating then decoding:

  # 1. Decode each individual dataset before concatenating (required for non-CF compliant and units not the same)
  # 551 ms ± 49.4 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
  %timeit -n 3 ds_decode_before = xcdat.open_mfdataset(["input/dummy/test1.nc", "input/dummy/test2.nc"])
  
  # 2. Decode after concatenating datasets (only for non-CF compliant and units the same)
  # 530 ms ± 35.2 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
  def decode_after():
      ds = xr.open_mfdataset(["input/dummy/test1.nc", "input/dummy/test3.nc"],decode_times=False)
      ds = decode_non_cf_time(ds)
      return ds
  %timeit -n 3 decode_after()

Summary of Changes

• Add optional boolean kwarg decode_times to open_dataset() and open_mfdataset()
  • Add conditionals to handle this kwarg when True or False
• Add optional callable kwarg preprocess to open_mfdataset()
  • Add _preprocess_non_cf_dataset() function to decode datasets' time values with non-CF compliant units before concatenating (handles cases where the datasets have the same time values and different time units, which would otherwise result in dropping of time values)
• Update decode_non_cf_time()
  • Rename from decode_time_units() to decode_non_cf_time()
  • Remove logic for checking cf compliance, which is now handled by _has_cf_compliant_time()
  • Fix incorrect start date for decoded time coordinates by forming them using offsets and reference dates, instead of reference date as the start point and a fixed pd.date_range()
    • Using pd.date_range() incorrectly assumes no gaps/missing data and that coordinate points started at the beginning of the month. It also did not handle calendar types correctly (e.g,. leap years), and would reset offsets at the beginning of the month or year if they weren't already.
  • Add decoding of time bounds
• Add utility function _split_time_units_attr() for splitting "units" attribute into units and reference date strings
• Update docstrings of methods
• Update test fixtures for correctness and readable syntax

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules

If applicable:

• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass with my changes (locally and CI/CD build)
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

[tomvothecoder]

Need to handle this still

[pochedls]

I think your catch on this issue and fix is really valuable. I read through the code and did not see problems, but I think merging this and testing it with a wide swath of CMIP (and non-CMIP) data will help to further evaluate / validate these methods.

[tomvothecoder]

I think your catch on this issue and fix is really valuable. I read through the code and did not see problems, but I think merging this and testing it with a wide swath of CMIP (and non-CMIP) data will help to further evaluate / validate these methods.

Thanks for the review! I'm glad to hear that the changes made sense to you. I also agree, we should stress test these changes using many different datasets.

I will merge this PR to your PR.