lava-nc / lava

A Software Framework for Neuromorphic Computing
https://lava-nc.org
Other
555 stars 143 forks source link

Enable int or None as run_condition for Process.run and Runtime.start #769

Closed tim-shea closed 3 months ago

tim-shea commented 1 year ago

Issue Number: #768

Objective of pull request: Add logic to create a RunSteps or RunContinuous in Runtime.start when user passes an int or None respectively. This allows users to just call process.run(5) or process.run() and get useful behavior, rather than always importing RunSteps/RunContinuous.

Pull request checklist

Your PR fulfills the following requirements:

Pull request type

Please check your PR type:

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

PhilippPlank commented 1 year ago

I am not sure if I like this change. Calling just <some_proc>.run() does not tell what is going to happen. Also <some_proc>.run(10) or <some_proc>.run(condition=10).

Maybe we can have parameters like: <some_proc>.run(steps=10) or <some_proc>.run(continuous=True) (although False would not really make sense, but I cannot think of a good way to start a continuous run without importing something)

Also blocking and non-blocking mode need to be specified for steps, which is currently part of the condition.

tim-shea commented 1 year ago

I am not sure if I like this change. Calling just <some_proc>.run() does not tell what is going to happen. Also <some_proc>.run(10) or <some_proc>.run(condition=10).

I agree that proc.run(10) is a bit weird and it would be better to add a param called steps. Agree to disagree that proc.run() is ambiguous. It does exactly what it says on the tin: runs.

Maybe we can have parameters like: <some_proc>.run(steps=10) or <some_proc>.run(continuous=True) (although False would not really make sense, but I cannot think of a good way to start a continuous run without importing something)

Also blocking and non-blocking mode need to be specified for steps, which is currently part of the condition.

Adding blocking also as an argument seems like it would resolve these.

Someone long ago made the decisions that RunSteps should default to blocking while RunContinuous should default to non-blocking, and while I find that not exactly obvious it has a bit of logic to it and I'm hesitant to change reasonable behavior without a good reason.

tim-shea commented 3 months ago

This isn't necessary. It was just a nice to have.