Skip to content

Fix subtraction of TimeDuration of, and from, Data #288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
sadielbartholomew merged 3 commits into from
Dec 21, 2021
Merged

Fix subtraction of TimeDuration of, and from, Data #288

sadielbartholomew merged 3 commits into from
Dec 21, 2021

Conversation

@sadielbartholomew
Copy link
Member

@sadielbartholomew sadielbartholomew commented Dec 20, 2021

Close #287 and add a test to capture it that bug, as well as a similar bug with a different traceback emering if the inverse is tried i..e the data subtracted from the timeduration (which I guess makes sense and should work, though isn't a very natural operation), namely (this report results from adding the new test lines to the master branch:

test_TimeDuration (__main__.TimeDurationTest) ... ok
test_TimeDuration_arithmetic (__main__.TimeDurationTest) ... ERROR
test_TimeDuration_bounds (__main__.TimeDurationTest) ... ok
test_TimeDuration_interval (__main__.TimeDurationTest) ... ok
test_TimeDuration_iso (__main__.TimeDurationTest) ... ok
test_Timeduration__days_in_month (__main__.TimeDurationTest) ... ok

======================================================================
ERROR: test_TimeDuration_arithmetic (__main__.TimeDurationTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_TimeDuration.py", line 487, in test_TimeDuration_arithmetic
    t2 = dt - da
  File "/home/sadie/cf-python/cf/timeduration.py", line 550, in __sub__
    return self._data_arithmetic(other, "__sub__")
  File "/home/sadie/cf-python/cf/timeduration.py", line 894, in _data_arithmetic
    return Data(dt, units=other.Units)
  File "/home/sadie/cf-python/cf/data/data.py", line 665, in __init__
    map(str, (x.year, x.month, x.day))
AttributeError: 'NotImplementedType' object has no attribute 'year'

----------------------------------------------------------------------

where commenting our the t2 = dt - da line and anything referring to t2 reveals the error Dan reported (for the full traceback it is:)

test_TimeDuration (__main__.TimeDurationTest) ... ok
test_TimeDuration_arithmetic (__main__.TimeDurationTest) ... ERROR
test_TimeDuration_bounds (__main__.TimeDurationTest) ... ok
test_TimeDuration_interval (__main__.TimeDurationTest) ... ok
test_TimeDuration_iso (__main__.TimeDurationTest) ... ok
test_Timeduration__days_in_month (__main__.TimeDurationTest) ... ok

======================================================================
ERROR: test_TimeDuration_arithmetic (__main__.TimeDurationTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_TimeDuration.py", line 488, in test_TimeDuration_arithmetic
    t3 = da - dt
TypeError: unsupported operand type(s) for -: 'Data' and 'TimeDuration'

----------------------------------------------------------------------

Fix suggestion

@davidhassell my suggestion is to change the implementation of __rsub__ to that shown, especially given the equivalently simple implementations of __radd__ and __rmul__, though because those are commutative and sub isn't, I would like to check that the idea is OK - perhaps relating to the non-commutativity __rsub__ must be done differently? If so, happy to change the approach, but something about the current implementation certainly isn't working and needs to be fixed and/or changed, and my suggestion does work at least...

@sadielbartholomew sadielbartholomew added the bug Something isn't working label Dec 20, 2021
@sadielbartholomew sadielbartholomew self-assigned this Dec 20, 2021
@sadielbartholomew sadielbartholomew changed the title Fix timeduration sub i287 Fix subtraction of TimeDuration of, and from, Data Dec 20, 2021
@davidhassell
Copy link
Collaborator

davidhassell commented Dec 20, 2021 •
edited
Loading

Thanks, Sadie - I'll have a look tomorrow.

@sadielbartholomew
Copy link
Member Author

sadielbartholomew commented Dec 21, 2021

CI test failures are both due to a lack of test_file.nc, so unrelated. I have noticed that this file is missing often locally too (I have in the test directory a copy I created to easily re-create it for these occasions 🙂 ). Is that file meant to be generated by the test set-up, David? If not it has gone AWOL so we should commit it back to the test directory.

davidhassell
davidhassell approved these changes Dec 21, 2021
View reviewed changes
Copy link
Collaborator

@davidhassell davidhassell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Sadie, looks good, thanks.

@sadielbartholomew sadielbartholomew merged commit 4fe10f3 into NCAS-CMS:master Dec 21, 2021
@davidhassell davidhassell added this to the 3.12.0 milestone Jan 4, 2022
@sadielbartholomew sadielbartholomew mentioned this pull request Jan 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidhassell davidhassell davidhassell approved these changes

Assignees

@sadielbartholomew sadielbartholomew

Labels

bug Something isn't working

Projects

None yet

Milestone

3.12.0

Development

Successfully merging this pull request may close these issues.

subtraction of cf.TimeDuration fails

2 participants

@sadielbartholomew @davidhassell