[PR] time type conversion to timedelta if timedelta64 is given

[lee1043]

Description

make sure time diff type is timedelta: in case it was timedelta64 convert it, so following .seconds line could run without error

• Closes [Bug]: add_missing_bounds not working when time type is numpy.timedelta64 #659

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)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (9ad91a0) to head (189e526).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #660   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines         1542      1544    +2     
=========================================
+ Hits          1542      1544    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pochedls]

@lee1043 - thanks for creating this issue and PR. Even though this was solved with use_cftime=True, I think this is an issue because this functionality should work with np.datetime64 axes.

I have a minor comment that I'm not really sure about the answer to (I'm curious about your opinion and @tomvothecoder's opinion).

[pochedls]

Suggested change

	diff = pd.to_timedelta(time.values[1] - time.values[0])
	# `cftime` objects only support arithmetic using `timedelta` objects, so
	# the values of `diffs` must be casted from `dtype="timedelta64[ns]"`
	# to `timedelta` objects.
	# `cftime` objects only support arithmetic using `timedelta` objects, so
	# the values of `diffs` must be casted from `dtype="timedelta64[ns]"`
	# to `timedelta` objects.
	diff = pd.to_timedelta(time.values[1] - time.values[0])

[pochedls]

Suggest putting the comment ahead of the code (as is done earlier in the code).

[pochedls]

I was initially averse to adding in a pandas converter here, because in a different project I've run into issues with this (the behavior of the datetime conversion is sensitive to the input array), but your pointing out that this is already done earlier in the code makes me way less worried about this.

In the code block you pointed out in the issue, this conversion is conditioned on the array being a time axis (not needed here) and the array containing cftime objects. It does seem like it might prevent problems to only do this conversion if necessary (i.e., add a conditional here).

But I am a little confused because the existing conditional is meant to apply to cftime-based axes (and here it is meant to apply to axes that do not have cftime objects).

I'm curious about:

• should there be a conditional to do this type conversion?
• should the conditional be applied if the object type(time.values[0]) == np.datetime64) or not contains_cftime_datetimes(xr.as_variable(time))?

[lee1043]

@pochedls thanks for the suggestion. I initially have coded as like below:

if isinstance(diff, np.timedelta64):
    diff = pd.to_timedelta(diff)

but this interrupted the GitHub Action CI check process for build, which was complaining that the function is too complicated. It seem like the CI process didn't like having more than a certain number of if layers.

I tested the code with a CMIP model data that has cftime-based time axis, and the same operation was running okay without errors, so was presuming it was okay with cftime-based axes. But I am open to any suggestion to make the code more robust.

[pochedls]

I like this... @tomvothecoder may be able to suggest some a way to pass the CI tests (or skip them for that line, e.g., here).

[lee1043]

I also like it but it does pass flake8 C901 with the error message of xcdat/bounds.py:507:5: C901 'BoundsAccessor._create_time_bounds' is too complex (11)

# type: ignore or #noqa: C901 (following the Stackoverflow) didn't help.

[lee1043]

I updated the code to have the if statement (fb90864) but as expected it fails flake8 checks. @tomvothecoder Any help will be appreciated.

[tomvothecoder]

I added #noqa: C901 and updated the comment inside _create_time_bounds() here: 4c290e7.

I can help with adding the unit test tomorrow, which you can hopefully try out in #655 too.

[tomvothecoder]

I pushed commit c6b314a to add test coverage for the new code. This PR should be good to go.

[lee1043]

@tomvothecoder thanks for the addition.

I am merging this PR.