Fix issue run at

[De-Cri]

Closes #1288

[mstimberg]

Apologies, I did not yet have time to have a look at the PR, but I will do so tomorrow !

[mstimberg]

This looks really good and very close to what I had in mind 😊 Unfortunately, it also breaks everything 😅

As explained in the comments, I get the appeal of the __eq__ approach, but I don't think it is the right solution here.

[mstimberg]

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?

[mstimberg]

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)

[mstimberg]

See my comment about __eq__ above

[mstimberg]

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)

[mstimberg]

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

[De-Cri]

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 :).
Edit: I also wanted to add that I'm the worst when it comes to coming up with names so I won't change them unless you have some recommendations.

[De-Cri]

I made some changes

[De-Cri]

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.

[mstimberg]

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'

[De-Cri]

Hi @mstimberg thanks for the suggestion, I'll get back to you with some more useful feedback as soon as possile.

[De-Cri]

``Hi @mstimberg , I was working on this issue but it seemed like I just miss typed self.clock.dt in the ClockArray meanwhile I set self._dt=float(dt) in the RegularClock class, after that the CUBA example worked. Then when I tried running some examples like test_clocks or test_networks it returned this error:
ERROR Brian 2 encountered an unexpected error. If you think this is a bug in Brian 2, please report this issue either to the discourse forum at http://brian.discourse.group/, or to the issue tracker at https://github.com/brian-team/brian2/issues. Please include this file with debug information in your report: C:\Users\ssamu\AppData\Local\Temp\brian_debug_1w2sxrlq.log Additionally, you can also include a copy of the script that was run, available at: C:\Users\ssamu\AppData\Local\Temp\brian_script_rpimb_ae.py Thanks! [brian2]
Traceback (most recent call last):
File "c:\Users\ssamu\OneDrive\Desktop\github-biran2\brian2\brian2\tests\test_clocks.py", line 62, in
restore_initial_state()
File "C:\Users\ssamu\OneDrive\Desktop\github-biran2\brian2\brian2\only.py", line 74, in restore_initial_state
defaultclock.dt = 0.1 * ms
^^^^^^^^^^^^^^^
File "C:\Users\ssamu\OneDrive\Desktop\github-biran2\brian2\brian2\core\clocks.py", line 321, in setattr
setattr(active_device.defaultclock, key, value)
File "C:\Users\ssamu\OneDrive\Desktop\github-biran2\brian2\brian2\groups\group.py", line 497, in setattr
raise TypeError(f"Variable {key} is read-only.")
TypeError: Variable dt is read-only.
Could you give me some feedback on how to handle this?
Edit: The main issue is that I changed the name of the dt in the regular clock because I need to create the dt here:

`    def __init__(self, dt, name="regularclock*"):
        self._dt = float(dt)
        self._old_dt = None  
        times = ClockArray(self)
        super().__init__(times, name=name)
        self.variables.add_array(
            "dt",
            dimensions=second.dim,
            size=1,
            values=float(dt),
            dtype=np.float64,
            read_only=True,
            constant=True,
            scalar=True,
        )`

I need the dt before the constructor of EventClock to set :self.variables["t"].set_value(self.times[0]) but I have to change the name or it triggers the setter and self.variables("dt") wouldn't find any attribute.
All this leads to any call such as defaultclock.dt would trigger the dt that is read only and not the _dt.

[De-Cri]

I also think that if we wanto to keep the _dt we should also add something to reset it aswell if needed.

[mstimberg]

Hi @Samuele-DeCristofaro. Uh, this is actually a very subtle thing and arguably a bug in Brian…

When you access an object attribute that is handled via Brian's variable mechanism (like dt), the getattr/setattr is directed through our own system. However, it has a dedicated check to make sure that if there is a corresponding property on the class, it will use the default mechanism (which goes through this property) and not use our variable access mechanism. In the Clock class, a property is used to forward the setting of dt through a special function (_set_dt) which keeps track of the previous value. This does not correctly work here, since it only checks whether the class itself (for the default clock: Clock) has this property, but in your case the property is in the parent class RegularClock. I think the easiest fix here for now is to avoid the inheritance. I'd suggest to simply rename RegularClock to Clock and to delete the inheriting Clock class. This is a less descriptive name for the regular clock, but it has the advantage that we don't have two classes with different names but identical functionality around.

[De-Cri]

Hi @mstimberg thanks, now it's all more clear, I really did not understand where the issue was 😅, I was also wondering if something like this would be useful aswell:

`    @check_units(dt_=1)
    def _set_dt_(self, dt_):
        self._old_dt = self._get_dt_()
        self.variables["dt"].set_value(dt_)
        self._dt=dt_
`

[De-Cri]

Hi @mstimberg, thanks for your tips. I'm making progress, but I'm still getting an error in test_network.py. The code seems to handle the restored dt value correctly - the assertion 'assert defaultclock.dt == 0.5 * ms' passes. However, I'm still getting an array comparison error with 'assert_equal(mon.t[:], [0, 0.5] * ms)'. The actual array is [0, 0.001] while the expected one is [0, 0.0005].
Any ideas what could be causing this mismatch between the monitor's times and the expected values, even though the clock's dt is restored correctly?:
Traceback (most recent call last):
File "c:\Users\ssamu\OneDrive\Desktop\github-biran2\brian2\brian2\tests\test_network.py", line 1879, in
t()
File "c:\Users\ssamu\OneDrive\Desktop\github-biran2\brian2\brian2\tests\test_network.py", line 1509, in test_dt_restore
assert_equal(mon.t[:], [0, 0.5] * ms)
File "C:\Users\ssamu\OneDrive\Desktop\github-biran2\brian2_env\Lib\site-packages\numpy\testing_private\utils.py", line 371, in assert_equal
return assert_array_equal(actual, desired, err_msg, verbose,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\ssamu\OneDrive\Desktop\github-biran2\brian2_env\Lib\site-packages\numpy_utils_init_.py", line 85, in wrapper
return fun(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^
File "C:\Users\ssamu\OneDrive\Desktop\github-biran2\brian2_env\Lib\site-packages\numpy\testing_private\utils.py", line 1056, in assert_array_equal
assert_array_compare(operator.eq, actual, desired, err_msg=err_msg,
File "C:\Users\ssamu\OneDrive\Desktop\github-biran2\brian2_env\Lib\site-packages\numpy\testing_private\utils.py", line 920, in assert_array_compare
raise AssertionError(msg)
AssertionError:
Arrays are not equal

Mismatched elements: 1 / 2 (50%)
Max absolute difference among violations: 0.0005
Max relative difference among violations: 1.
ACTUAL: Quantity([0. , 0.001])
DESIRED: Quantity([0. , 0.0005])

[mstimberg]

Hi @Samuele-DeCristofaro This seems to be because the time calculation in ClockArray uses Clock._dt which does not get stored/restored by the usual mechanism. Is it really necessary to have it, i.e. couldn't you use Clock.dt instead there? Maybe, if the reason is in the EventClock → Clock inheritance, going back to the previously discueed design of having an abstract BaseClock (with the two types of clocks inheriting from it) would be better?

[De-Cri]

Hi @mstimberg before changing the approach I tried to use the initial_dt only to set the dt the first time:

class ClockArray:
    def __init__(self, clock):
        self.clock = clock
        

    def __getitem__(self, timestep):
        return self.clock.dt * timestep


class EventClock(VariableOwner):
    def __init__(self, times, name="eventclock*"):
        Nameable.__init__(self, name=name)
        self.variables = Variables(self)
        self.variables.add_array(
            "timestep", size=1, dtype=np.int64, read_only=True, scalar=True
        )
        self.variables.add_array(
            "t",
            dimensions=second.dim,
            size=1,
            dtype=np.float64,
            read_only=True,
            scalar=True,
        )
        self.variables["timestep"].set_value(0)
        if isinstance(times, ClockArray):
            self.times = times  # Don't sort, don't check for duplicates
            self.variables.add_array(
                "dt",
                dimensions=second.dim,
                size=1,
                values=times.clock.initial_dt,
                dtype=np.float64,
                read_only=True,
                constant=True,
                scalar=True,
            )
        else:
            self.times = sorted(times)
            if len(self.times) != len(set(self.times)):
                raise ValueError(
                    "The times provided to EventClock must not contain duplicates"
                )
        self.variables["t"].set_value(self.times[0])

, when it was time to set up the t, and now we can simply always use dt even if it's not the cleanest way, in the test_network, it didn't give any errors but it took like 30 s to simulate, then I started making some adjustments in the network file, specifically in the nextclock and the run function, and it's getting faster, now it takes 14s by making small adjustments. I can try to optimize things even more but I don't know how much faster it can get. And it seems very tricky 🥲. Could you provide any feedback on how to handle this?

[De-Cri]

I also forgot to mention that with this new clock implementation, I tried using the old network file and the test_network worked perfectly without any changes or errors in the expected output. This means that there are no major issues in the clocks. I also feel like we’re very close to finishing this PR. I'm doing some more testing to figure out what's causing these issues.

[mstimberg]

Hi @Samuele-DeCristofaro I feel that this is not the right way: if the EventClock's __init__ file needs to have a special case for how it is called from the subclass (with a ClockArray for times), then there is something wrong in the overall inheritance structure. If Clock (i.e. RegularClock) and EventClock both are independent (inheriting from the same base class), then the code will look much simpler and easier to understand, we won't even need ClockArray anymore, since we can directly implement the t access differently in both cases.

[De-Cri]

Hi @mstimberg, alright I will then apply these changes requested, but what about the network file is there something to avoid using that could lead to some performance issues as I said before.

[mstimberg]

Please update the PR first. I did not quite understand the performance issue, and it is much easier for me to simply try out the code and see for myself. Thanks!

[De-Cri]

PR updated. I'm sorry, I should have updated the PR earlier, but I'm trying to manage on my own, I took the work and I don't want to ask for help too often. I also have another implemetation of the network file that simulates in 14 s, instead of the current updated which does it in 30s. I think the issue is calling the variables too often.

[mstimberg]

Thanks for updating the PR. I tried it out, and the performance of some test cases is indeed much worse than before. A good/extreme example is the test_network_different_clocks test from test_network.py, because it does not really do anything during a run, but simulates one clock for 1 million, and another one for 100000 time steps. If you run this test with a profiler (e.g. Python's built-in cProfile, you can visualize the results with tools like snakeviz), you see that a lot of time is spent in ClockArray.__getitem__. The reason is that this accesses self.clock.dt, which creates a new Quantity object for each access to provide the values with physical units. Since the value is only used to set the t value via set_value, you do not actually need units, a faster access would therefore be to use self.clock.dt_ or self.clock.variables['dt'].get_value().item(). A similar but more subtle issue occurs in the same_time function because it uses hasattr(..., 'dt'): this will actually get the value of dt (i.e., including units) to see whether it raises an error... A faster check would be to use isinstance, but by implementing this function separately for EventClock and Clock, you'd also have one check less (since you know for the class itself whether it has a dt or not). I'd make things "reasonably" fast for now by fixing these big issues, but it is not necessary to make as fast as it was previously. This would require keeping direct references to the arrays storing t, dt, and timestep around instead of going through variables[...],get_value() every time, but this approach is also a bit more error-prone.

[De-Cri]

PR updated again, please let me know if more changes are needed.
Edit: The PR needs little changes, I had some issues with the testing this morning, I'm going to fix them and make another commit.

[De-Cri]

PR updated ready to be checked, I also added some simple checks to test_clocks.py for the new event_clock, let me know if some checks could be added up or if it's not needed at all. As you expected this new implementation is slightly slower then the initial one but I don't think there are any big issues now. I'm sorry if this took too long I'm having a busy-rough time.

[De-Cri]

Also if we manage to merge this PR, I would be open to help more with everything if needed.

[mstimberg]

Hi @Samuele-DeCristofaro. I think this looks good so far, we'll keep the final performance tweaking for the end. The new test looks good in general, but I'd tweak the values a bit to have irregularly spaced events (which is kind of the point of EventClock ;-) ), and also add a test that combines several clocks in a run.
Finally, we need to add the run_at method! This should be almost trivial now, we can have one general run_code function (not sure what the best name would be…) that is basically identical to the current run_regularly function, but does not allow the dt argument, and a run_at function that has a times argument instead of dt and clock, and calls run_code with clock=EventClock(times).
It would be more consistent if run_regularly would only have a dt argument and no clock (in the same way that run_at only has times), but that might break someone's code…

[De-Cri]

Hi @mstimberg, PR updated! I just wanted to ask what could be added specifically to the new test, I changed how the duplicates in the EventClock were checked and now it works for Quantity times aswell, since before I was using set but they are not hashable.
I also made some other changes if you have time, a quick check would be really helpful. Thanks!

[mstimberg]

Hi @Samuele-DeCristofaro. Sorry, it took me a while to get back to this PR. Regarding the Quantity support ­– I realized that the EventClock (and the times argument of run_at) should only support a Quantity with the correct units to be consistent with how we handle units elsewhere. Therefore, there should be a times = Quantity(times) (to support lists of quantities) and a fail_for_dimension_mismatch(...) somewhere at the top.

Regarding the tests: I think your tests cover the essentials, but instead of the network_operation, I'd use the NameLister that is used elsewhere in test_network. E.g. if you run a network with

 x = NameLister(name="x", dt=0.1 * ms, order=0)
 y = NameLister(name="y", clock=EventClock(times=[0*ms, 0.5*ms, 1.5*ms]), order=1)

It should give `"xyxxxxxyxxxxxxxxxxyxxxx" for a 2 second run.

[mstimberg]

I find this name a bit confusing, how about run_on_clock?

[De-Cri]

Hi @mstimberg,
I've updated the PR let me know if anything else needs fixing. The only issue I'm running into is that sometimes the new test fails because the last "x" update isn't included if the run stops just before it. I suspect this is due to the timing alignment and I thought maybe to just extend the duration of the run just a bit.
Would you suggest a way to fix this while keeping the test structure consistent?
Thanks!:
File "C:\Users\ssamu\OneDrive\Desktop\github-brian2\brian2\brian2\tests\test_clocks.py", line 111, in test_combined_clocks_with_run_at
x_count == expected_x_count
AssertionError: Expected 5 x's, got 4

[mstimberg]

Hi @Samuele-DeCristofaro. Thanks, the changes look good in general. Can you please mark the pull request as "Ready for review"? I will then have a final look.

I've updated the PR let me know if anything else needs fixing. The only issue I'm running into is that sometimes the new test fails because the last "x" update isn't included if the run stops just before it. I suspect this is due to the timing alignment and I thought maybe to just extend the duration of the run just a bit.

A test that fails sometimes is a broken test, we definitely need to fix this. The behaviour should be deterministic, it should always run the exact same number of steps for a given time. But I see where the problem is coming from: at the end of each time step, we advance the clocks. At the end of a simulation with regular clocks, all clocks will therefore have the timestep/time that is > the requested run time – this is the check that we are doing to see whether we should stop the current run. For the event clock, we are never going beyond the events that it has, so it cannot be set to the next time step. Actually, this means that the current EventClock is also broken for repeated runs, after doing one run that exhausts all events, another run will lead to an error.
I did not try it out, but I think the easiest fix would be to automatically at an additional event at np.inf*ms at the end of the event list. This way, after exhausting all events, the clock will have t=∞, and it should be ignored.

[De-Cri]

HI @mstimberg , I tried your tip but the test it's still not always working 😭 . I marked the PR as "Ready for reveiw".

[mstimberg]

Hi @Samuele-DeCristofaro The problem was that i_end was off by one, i.e. the added inf was basically ignored. I fixed this and added another test, things seem to work for now. I'll have a closer look tomorrow!

[De-Cri]

Got it! Thanks for the help! 😃 I got confused because sometimes the test worked, so I thought there was something wrong with the run method. But I was looking at the wrong place 😅

[mstimberg]

Looks good, we are getting there! I've left several nit-picking comments, but those are mostly minor issues. The main remaining task is to get C++ standalone support in place (but I can take care of that if you prefer), and to mention the new features in the documentation.

[mstimberg]

I feel it is a bit confusing that the Quantity array gets converted into a list here. But maybe more importantly, I think we will need to create a times array with Brian's add_array mechanism to get things to work with C++ standalone mode.

[De-Cri]

I created the times array, does it make sense to use a numpy array instead of a list there or maybe just sorting manually.

[mstimberg]

When you create the Brian variable times below, it will already be stored internally as a numpy array. To not duplicate things, I'd not store self._times at all, e.g. you could do times = sorted(times) and times.append(np.inf*ms) before passing values=times to the add_array function.

[mstimberg]

The error message will be slightly less useful, but I think I'd prefer a simple

if len(self._times) != len(set(self._times)):
  raise ValueErrror(...)

[De-Cri]

I've already tried your solution before but set uses eq methods to compare the times so it returns an error with Quantity types.

[mstimberg]

Hmm, I see. It's not about __eq__ but about Quantity not being hashable. But we could move this test to use the numpy array we create later, or use len(set(np.asarray(times))) or similar.

[De-Cri]

I'm very happy we are getting there !, I think I can work on the cpp standalone part as well, and I'll fix these minor issues ASAP, I have no clue about what happened to those quotes 🥲.

[mstimberg]

Thanks for the updates, I made a few more minor comments. But as I said earlier, the big thing to do now would be the C++ standalone support – let me know if you need further guidance!

[mstimberg]

left-over print

[mstimberg]

I did not mean to only remove the comment, but also to actually care about the exact output in the test below 😊

[mstimberg]

Remove print (but check full string ;-) )

[mstimberg]

Hmm, I see. It's not about __eq__ but about Quantity not being hashable. But we could move this test to use the numpy array we create later, or use len(set(np.asarray(times))) or similar.