Skip to content
This repository was archived by the owner on Jun 11, 2025. It is now read-only.

feat: Add AdapterCheckpoint class to run simulations in steps#921

Closed
filimarc wants to merge 8 commits intodevfrom
feature/handle_checkpoints
Closed

feat: Add AdapterCheckpoint class to run simulations in steps#921
filimarc wants to merge 8 commits intodevfrom
feature/handle_checkpoints

Conversation

@filimarc
Copy link
Contributor

@filimarc filimarc commented Apr 18, 2025

Describe the work done

For some devices may be necessary to flush the results before simulation ends. Here is proposed a class that managed a list of checkpoints, extracted from devices' configuration. All Device classes now have a checkpoint attribute that AdapterCheckpoint can inspect before simulation start.

Tasks

  • Added tests
  • Updated documentation

📚 Documentation preview 📚: https://bsb--921.org.readthedocs.build/en/921/

@github-actions github-actions bot added the feat label Apr 18, 2025
Copy link
Contributor

@Helveg Helveg left a comment

Choose a reason for hiding this comment

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

The BSB is in a stable and mature phase now, new features best pass through a phase of technical discussion with a write up of the use-cases and an agreed upon technical proposal before moving to the implementation phase.

@drodarie @filimarc I don't understand the use case or context of this PR so I can't really provide review, can we write up the use case for this in an issue before continuing with this one?

I think it's best in general that we first write an issue for something, tag each other to get each other's input, come up with a proposal we agree on, and then someone can start on implementation.

What I think I can extract from context:

  • There are devices that need to run a callback (let's say "flush") at certain times
  • The devices need to be able to schedule all their callbacks
  • The simulation control should pause and run their callbacks as scheduled

If I have understood the objective correctly I have some issues with the current implementation:

  • CMIIW, but it seems like the user has to change the simulation run code in order for this to work? That's not acceptable because devices that depend on this interface might be used without this altered setup, which would break things. This feature would need to work in such a way that as soon as a device is added that uses checkpoints, the simulation control is adjusted.
  • The base DeviceModel is too coupled to this feature. If we add this I think the entire feature should be optional and the DeviceModel class should be left as-is; at most we can add a get_checkpoint noop placeholder that returns []. Any other logic should be moved to opt-in mixins, so that each device can implement their own checkpoint logic based on their needs.

Then some implementation details, that we can discuss only after we decide on the proposal:

  • The checkpoints are now all known, but then it seems that they are sorted many times, why not sort them once and store them sorted?
  • Since all checkpoints are known, what is this suitable_step needed for exactly?
  • There seems to be a conflation between the simulator timestep resolution and this feature. I didn't go into enough detail to figure out the problem, but: the resolution matters only for accuracy of the numerical solvers vs. computational load; it shouldn't be tied to how we start-stop pieces of the simulation, like we do here. The resolution is simply a setting we set on the underlying simulator; e.g. it's totally fine to set a resolution of 0.1ms, and then tell the BSB to simulate 1s in pieces of 2ms, the underlying simulator will then simulate those many pieces of 2ms still with a resolution of 0.1ms, returning to the BSB start-stop loop in Python every 2ms. They are different things

@filimarc
Copy link
Contributor Author

Hi,
thank you for your comments.
I am sorry if i started to implement without proposal approval, i will be happy to change things to make a better design/implementation.
The need to add this feature raised with the development of a device for integrate LFPy analysis, the issue related i think is: #792 . For an use case we can look at the draft PR where we are implementing an LFP device: dbbs-lab/bsb-neuron#29 .
For the moment it is working without breaking the usage.
To reply on your points:

  1. We already provide a run implementation for bsb-neuron, bsb-nest and bsb-arbor. So we can adjust our adapter to integrate the option to use checkpoints and flush if needed, without additional coding by the user. In the documentation i've added that part to explain how to use this object in an adapter.
    Actually in our Adapters we already process the simulation in steps in order to tick progression for listeners, so i thought to add in that workflow an option to trigger if checkpoints were provided (if not the simulation keep going as it is).
    so for give a design of what i thought:
    in run():
    create AdapterProgress that create an iterator for listeners ticks.
    create AdapterCheckpoint -> investigate all devices about checkpoints, create an iterator on all checkpoints.

    for on AdapterProgress:
    if AdapterCheckpoint.status return true:
    do the flush
    In this case if no device has implemented checkpoints the AdapterCheckpoints return [] and we never have intermediate flush.
    Maybe if we want to remove the if clause inside the for loop i can try to refactor in order to do a loop from checkpoint to checkpoint and inside do another loop for every progression step.

  2. I decided to implement this directly in the DeviceModel so users could access the feature on any device without needing to write code. But I understand if you'd prefer not to modify the class. Are you suggesting creating a mixin class to inherit this feature instead?

@Helveg
Copy link
Contributor

Helveg commented Apr 28, 2025

Thanks, this addresses my concerns, and yes I'd prefer this the base DeviceModel implements only an optionally extensible interface whose default behavior is "no checkpoints", and that any logic that lets users add certain specific checkpoint behaviors, be provided as a mixin :)

For the remaining 3 points, shall we discuss in the laboratory? Which days are you physically present?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants