-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support iterating with user-supplied callbacks #115
Conversation
df92afc
to
0deff40
Compare
ef42180
to
e951b10
Compare
@MiguelPobleteCazenave would you mind taking a first pass in reviewing? Happy to do a second after the first round. |
sure, i'll give it a look later today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I just have one question maybe thinking forward. Wouldn't be desirable to pass more than just one argument for the callback (in case you want to have different versions of the other models, e.g., different levels of access targeted, different regions or in transport, different types of vehicles targeted)?
Ah, good call. I will add this now. The user can also do this in a bunch of other ways, because the callback function will usually be defined in the same scope where myparam1 = 10.2
def cb(scenario):
# Use a global variable
global myparam1
# Retrieve data stored with the callback
myparam2 = cb.data['myparam2']
# Read a file
with open('myparam3.txt') as f:
myparam3 = f.read()
# do some calculations
return convergence
# Store data with the callback after it has been defined
cb.data = {'myparam2': 23.5}
# Callback will have access to myparam[123]
scenario.solve(callback=cb) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this looks great! One in-line comments in tests.
More broadly, my first thought is whether the callback being defined as a function is sufficient. It seems to me that there will likely need to be a decent amount of information based back (and perhaps forth) between the message model run and callback model run. As is, the iteration number can be queried directly by the callback.
Perhaps it is fine to keep as is for now, but my first thought would be to prefer more a class-type implementation, e.g.,
class MyCallBack:
def __init__(self, scenario, **kwargs):
self.scenario = scenario
def __call__(self, **kwargs):
# do whatever is needed here
But I think this is not mutually exclusive to the current implementation, more a question of what type of implementation we suggest to users..
|
||
# Warning is raised because 'return False' is commented above, meaning | ||
# user may have forgotten any return statement in the callback | ||
message = (r'solve\(callback=...\) argument returned None; will loop ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be checking that the message is the same as that defined in core.py
. Is it worth making this a top-level defined string there, e.g., CALLBACK_WARNING=..
and here setting message = core.CALLBACK_WARNING
? As is, it seems like if someone later edits the callback warning without touching this code as well, the test may not behave as expected.
Per our in person conversation, we will leave test as is and let future devs deal with it if it becomes an issue. |
This PR adds a feature to
Scenario.solve()
iteratively, with some user-supplied callback.This is to generalize and support model variants like MESSAGE-Access and MESSAGE-Transport.
PR checklist: