Conversation
|
n.b. this will supercede some work in #175 |
pearsonca
force-pushed
the
static_binding
branch
from
259cdaf to
7039c2d
Compare
jc-macdonald
left a comment
jc-macdonald
left a comment
There was a problem hiding this comment.
mostly looks good to me, did raise a couple of points.
pearsonca
force-pushed
the
static_binding
branch
from
7039c2d to
2fe4ea7
Compare
TimothyWillard
left a comment
TimothyWillard
left a comment
There was a problem hiding this comment.
Already reviewed most of the implementation previously so that's good. I would like this PR to be more focused though, seems like there are changes coming in that aren't relevant to the original goal here. I also have a preference for squashing these commits when merging, no reason for this single change to be split over 10+ commits.
There was a problem hiding this comment.
This guide was originally designed to be more technical, and this looks like a lot of details about conceptual elements are being introduced. Maybe not necessarily in this PR, but I think there should be a new section in the documentation discussing model words vs representations vs implementations rather than inlining it with technical implementation. I also foresee having to add other conceptual elements in the future anyways so there will need to be a home for this I think.
There was a problem hiding this comment.
k - new PR? i think we need a place for the conceptual linkage somewhere, but open to where it goes.
jc-macdonald
left a comment
jc-macdonald
left a comment
There was a problem hiding this comment.
I agree with @TimothyWillard regarding separation of concerns/PR hygiene + squashing. Otherwise good
Added a way to "bind" parameters to a `SystmABC` which will return the `_stepper` of said system wrapped in a partial of the given parameters. - Added private `_checked_partial` utility function as a thin wrapper around `functools.partial` but with custom validation. - Implemented `SystemABC.bind` which is itself a thin wrapper around `_checked_partial`. - Modified the "Implementing Custom Engines and Systems" documentation to include conceptual descriptions of systems/engines.
TimothyWillard
force-pushed
the
static_binding
branch
from
cba653d to
450022c
Compare
|
@pearsonca I have squashed this PR into one commit as you requested. |
Added missing `state_change` configuration to wrapper systems/engines. Likely required following ACCIDDA/flepimop2#179.
Added missing `state_change` configuration to wrapper systems/engines. Likely required following ACCIDDA/flepimop2#179. Helps to address #16, but does not close it due to an issue with `op_engine`'s implementation, see ACCIDDA/op_engine#15.
Added missing `state_change` configuration to wrapper systems/engines. Likely required following ACCIDDA/flepimop2#179. Closes #16.
Added missing `state_change` configuration to wrapper systems/engines. Likely required following ACCIDDA/flepimop2#179. Closes #16.
Added missing `state_change` configuration to wrapper systems/engines. Likely required following ACCIDDA/flepimop2#179. Closes #16.
3 participants
Added a way to "bind" parameters to a
SystmABCwhich will return the_stepperof said system wrapped in a partial of the given parameters._checked_partialutility function as a thin wrapperaround
functools.partialbut with custom validation.SystemABC.bindwhich is itself a thin wrapper around_checked_partial.to include conceptual descriptions of systems/engines.