Fix issue run at #1602
Fix issue run at #1602
Conversation
|
Apologies, I did not yet have time to have a look at the PR, but I will do so tomorrow ! |
brian2/core/clocks.py
Outdated
| def __init__(self, times, name="eventclock*"): | ||
| Nameable.__init__(self, name=name) | ||
| self.variables = Variables(self) | ||
| self.times = times |
There was a problem hiding this comment.
We should probably sort these times (given that we assume them to be sorted later). Also, maybe include a check that it does not contain the same time twice?
brian2/core/clocks.py
Outdated
| def __lt__(self, other): | ||
| return self.variables["t"].get_value().item() < other.variables["t"].get_value().item() | ||
|
|
||
| def __eq__(self, other): |
There was a problem hiding this comment.
While I get the appeal of using __eq__ here, I don't think it is the right approach – also, it breaks everything :) The reason is that we don't want to consider two Clocks to be equal just because their time is the same. E.g. there are places where we check whether two objects use the same clock, and those would always return True at the start of a simulation when those clocks are at 0ms. Also, we have places where we put clocks into a set, and Python objects that have __eq__ but not __hash__ are not hashable (and the hash of an object is not allowed to change and needs to be consistent with __eq__ so we cannot simply add a __hash__ function. I think the best approach would be to add a specific function to compare them, say same_time(self, other)
brian2/core/network.py
Outdated
| or abs(time - min_time) / min(minclock_dt, dt) < Clock.epsilon_dt | ||
| ) | ||
| for clock in self._clocks | ||
| if clock == minclock |
There was a problem hiding this comment.
See my comment about __eq__ above
brian2/core/network.py
Outdated
| ] | ||
| minclock, min_time, minclock_dt = min(clocks_times_dt, key=lambda k: k[1]) | ||
|
|
||
| minclock = min(self._clocks) |
There was a problem hiding this comment.
While this (i.e. defining __lt__ and __le__) is less of an issue than the equality check, for consistency/readability we should probably also have an explicit function here, or simply use something along the lines of min(self._clocks, key=lambda c: c.t)
| return minclock, curclocks | ||
|
|
||
There was a problem hiding this comment.
Nothing to worry about before the PR is finalized, but have a look at our documentation on how to enable pre-commit so that it will make sure that the source code is formatted consistently: https://brian2.readthedocs.io/en/stable/developer/guidelines/style.html#code-style
There was a problem hiding this comment.
RegularClock and EventClock should be added to __all__ as well.
|
Thanks a lot for taking the time to guide me through my mistakes, I must have overlooked the mention of the eq, I'll get to work and get back to you with news soon :). |
|
I made some changes |
|
Hi @mstimberg, I've made all the changes I think are needed, but I'm not too sure if they are correct, looking for your feedback. |
|
Hi @Samuele-DeCristofaro. I did not go through your updated code in detail yet, but I'd suggest that you'd try running a simple example that uses your branch. If I do this for the CUBA example, I get an error which strongly suggests that there is still something to fix 😊 For more thorough testing, please look at the documentation on how to run the test suite: https://brian2.readthedocs.io/en/stable/developer/guidelines/testing.html#running-the-test-suite Here's the full error I am getting: Traceback (most recent call last):
File "/home/mstimberg/git/brian2/examples/CUBA.py", line 17, in <module>
from brian2 import *
File "/home/mstimberg/git/brian2/brian2/__init__.py", line 91, in <module>
from brian2.only import *
File "/home/mstimberg/git/brian2/brian2/only.py", line 59, in <module>
set_device(all_devices["runtime"])
File "/home/mstimberg/git/brian2/brian2/devices/device.py", line 684, in set_device
_do_set_device(device, build_on_run, **kwargs)
File "/home/mstimberg/git/brian2/brian2/devices/device.py", line 698, in _do_set_device
active_device.activate(build_on_run=build_on_run, **kwargs)
File "/home/mstimberg/git/brian2/brian2/devices/device.py", line 378, in activate
self.defaultclock = Clock(dt=0.1 * ms, name="defaultclock")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/mstimberg/git/brian2/brian2/core/clocks.py", line 300, in __init__
super().__init__(dt, name)
File "/home/mstimberg/git/brian2/brian2/core/clocks.py", line 186, in __init__
super().__init__(times, name=name)
File "/home/mstimberg/git/brian2/brian2/core/clocks.py", line 76, in __init__
self.times = sorted(times)
^^^^^^^^^^^^^
File "/home/mstimberg/git/brian2/brian2/core/clocks.py", line 70, in __getitem__
return self.clock.dt * timestep
^^^^^^^^^^^^^
File "/home/mstimberg/git/brian2/brian2/core/clocks.py", line 257, in <lambda>
fget=lambda self: Quantity(self.dt_, dim=second.dim),
^^^^^^^^
File "/home/mstimberg/git/brian2/brian2/core/clocks.py", line 245, in _get_dt_
return self.variables["dt"].get_value().item()
~~~~~~~~~~~~~~^^^^^^
File "/home/mstimberg/git/brian2/brian2/core/variables.py", line 1646, in __getitem__
return self._variables[item]
~~~~~~~~~~~~~~~^^^^^^
KeyError: 'dt' |
|
Hi @mstimberg thanks for the suggestion, I'll get back to you with some more useful feedback as soon as possile. |
No description provided.