-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add some convenience to simulation creation. #1904
Add some convenience to simulation creation. #1904
Conversation
Encapsulates 80% of cases: ```cxx rec = recipe() ctx = make_context() dec = partition_load_balance(rec, ctx) sim = simulation(rec, dec, ctx) ``` is now written as ```cxx rec = recipe() sim = simulation(rec) ``` In python we use keyword args to allow both to be specified separatly.
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.
Nice! I checked only the Python impact
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.
Noice, a few requests.
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.
No objections 👍
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.
Some minor things.
decomp = arbor.partition_load_balance(recipe, context) | ||
sim = arbor.simulation(recipe, decomp, context) | ||
ctx = arbor.context("avail_threads") | ||
sim = arbor.simulation(recipe, context=ctx) |
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.
sim = arbor.simulation(recipe, context=ctx) | |
sim = arbor.simulation(recipe, arbor.context("avail_threads")) |
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.
Robin requested the exact opposite ;)
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.
I'll argue my case:
- If argument order changes, which it just did, using explicitly named arguments obviates refactoring on the call site.
- It's more explicit, although
arbor.context
and the variable namectx
make it clear enough. It's not that WET either. You'll find many call sites have patterns liketree=tree, axe=axe, chop=chop
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.
The main thing I care about is that as of The Swap(tm) context=
can be left out. I'd have peace with
sim = arbor.simulation(recipe, context=ctx) | |
sim = arbor.simulation(recipe, ctx) |
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.
So, I'll let you two battle that out, I'd chosen @brenthuisman 's suggestion and only change due to Robin's wishes.
Less typing is almost always to be preferred.
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.
Fine with me :)
- Mention arbor-contrib in a few relevant places - Correct some fallout from arbor-sim#1904 - A new hardware and profiling tutorial, and covers things moved out of other examples in arbor-sim#1904 - Various other documentation fixes
Encapsulates 80% of cases:
is now written as
In python we use keyword args to allow
both to be specified separatly.
Partially fixes #1862