JesusEV / nest-simulator

The NEST simulator
GNU General Public License v2.0
1 stars 0 forks source link

Separate creating and writing eprop history #9

Closed akorgor closed 5 months ago

akorgor commented 5 months ago

This PR alleviates the inconsistency between the functions write_surrogate_gradient_to_history, write_error_signal_to_history, write_learning_signal_to_history, and the imprecise names of the first two. The first two write the corresponding value to the history and create the history entry, whereas the third writes to existing entries.

Note that write_update_to_history and one of the overloaded write_firing_rate_to_history also create history entries, which can be maybe specified in the function's name.

While this new logic disentangles the creation and writing of the history entries, the old logic is more efficient. Thus, I would be happy with either solution.

JesusEV commented 5 months ago

@akorgor with this change write_surrogate/error_signal_gradient_to_history does not play a dual role that forces it to be called first than the other write_ functions, for no reasons other than technical ones. I believe this particular nuance might be somewhat restrictive, and the change could ease future expansions or the reuse of the framework. In that regard, I consider it a positive update. However, as you mentioned, this adjustment introduces an additional call to get_eprop_history. If the impact of this addition is minimal, I think the benefits outweigh the costs.

It would be good if you confirm that last aspect.

Have you considered the name add_new_eprop_history_entry ?

akorgor commented 5 months ago

Have you considered the name add_new_eprop_history_entry ?

I like the idea, maybe a bit more precise would be append_new_eprop_history_entry or emplace_new_eprop_history_entry?

akorgor commented 5 months ago

However, as you mentioned, this adjustment introduces an additional call to get_eprop_history. If the impact of this addition is minimal, I think the benefits outweigh the costs.

It would be good if you confirm that last aspect.

In this image, each point corresponds to a simulation of 100 iterations of the sine waves task. The slowdown is systematic but marginal.

image

JesusEV commented 5 months ago

@akorgor From your plot I think the compromise seems reasonable. Regarding naming: any of those looks fine.