Skip to content
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.

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

Support comparing date-like objects #629

Merged
merged 4 commits into from
May 26, 2022
Merged

Support comparing date-like objects #629

merged 4 commits into from
May 26, 2022

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented May 18, 2022

Discovered this bug while working on holoviz/lumen#276

The Comparator class didn't support datetime.date, datetime.datetime and np.datetime64 object. This meant that, if a parameter value was set to its old value, Param would still see it as a new value and would let potential watchers be executed, while they should not. This caused problems for Panel bidirectional links which don't check for equality but rely on Param.

I see that other types aren't supported, like Numpy Arrays, Pandas DataFrame, or frozensets. If more types should be added, let's come up with a list and add them in another PR.

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2022

Codecov Report

Merging #629 (46d6eb9) into master (d960230) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #629      +/-   ##
==========================================
+ Coverage   82.91%   83.07%   +0.15%     
==========================================
  Files           5        5              
  Lines        3073     3084      +11     
==========================================
+ Hits         2548     2562      +14     
+ Misses        525      522       -3     
Impacted Files Coverage Δ
param/parameterized.py 83.84% <100.00%> (+0.51%) ⬆️
numbergen/__init__.py 80.37% <0.00%> (-0.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d960230...46d6eb9. Read the comment docs.

@maximlt
Copy link
Member Author

maximlt commented May 19, 2022

@philippjfr I've seen you added the Comparator class, in 6fe66a8 I added an identity check to short cut the equality comparisons. This I think would have fixed the issue I was having with bidirectional links in Panel, since the same attribute object is shared between the linked Parameterized instances.

However this test is failing because the code did not handle any other type than the ones declared in the equalities dictionary.

  _____________ TestWatch.test_triggered_when_unchanged_complex_type _____________
  
  self = <tests.API1.testwatch.TestWatch testMethod=test_triggered_when_unchanged_complex_type>
  
      def test_triggered_when_unchanged_complex_type(self):
          def accumulator(change):
              self.accumulator += 1
      
          obj = SimpleWatchExample()
          obj.param.watch(accumulator, 'a')
          subobj = object()
          obj.a = subobj
          self.assertEqual(self.accumulator, 1)
          obj.a = subobj
  >       self.assertEqual(self.accumulator, 2)
  E       AssertionError: 1 != 2

param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
@philippjfr
Copy link
Member

I've removed the identity check, not because I think we shouldn't do it but because it's a change in behavior and we need to consider it carefully. Please open a new issue or PR to discuss further.

@philippjfr
Copy link
Member

MacOS tests passed but coveralls errored for some reason. Merging.

@philippjfr philippjfr merged commit ad7d22a into master May 26, 2022
@philippjfr philippjfr deleted the compare_datetime branch May 26, 2022 16:07
@maximlt
Copy link
Member Author

maximlt commented May 30, 2022

This was a codecov issue (codecov/codecov-action#745) that is now resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants