Skip to content

Conversation

rjplevin
Copy link
Collaborator

@rjplevin rjplevin commented Jun 8, 2019

Made it possible to call sim = Simulation() and use the new API to populate sim.

@rjplevin rjplevin requested a review from davidanthoff June 8, 2019 01:52
@davidanthoff davidanthoff requested a review from lrennels June 8, 2019 02:46
Copy link
Collaborator

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

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

I like the idea, but will let @lrennels do a proper review of the code :)

@lrennels
Copy link
Collaborator

lrennels commented Jun 8, 2019

What’s the use case here? Doesn’t the new API work about the same but by deep copying the existing sim and then adding a few characteristics like models and N?

@rjplevin
Copy link
Collaborator Author

rjplevin commented Jun 8, 2019

This was in response to James' issue. It occurred to me that he could allocated an empty sim and use the functional API to add to it in each component's file. We might streamline the API as we discussed.

Alternatively, we could modify @defsim so it also works on a pre-allocated Simulation. This would provide the "incremental defsim" we talked about today. It would basically do what I suggested above, but it would be consistent with the existing approach. In fact, it would be a handy way to modify an existing Simulation, say, returned from Mimi.get_sim(). Maybe call it @update_sim instead.

@lrennels
Copy link
Collaborator

lrennels commented Jun 8, 2019

Ah ok so this is in line with our conversation at the end today, not just the API we had on the board.

@davidanthoff
Copy link
Collaborator

We could also generally think about support for merging simulations? Either via merge(s1, s2), or maybe even something like s1 + s2? Although I kind of like the first option better, because it is more explicit that this operation is not commutative.

@rjplevin
Copy link
Collaborator Author

rjplevin commented Jun 8, 2019

Yes, merge(s1, s2, ...) seems good. We could also support merge!(s1, s2, ...), as for Dicts. Or We define the merge operation as a constructor Simulation(sim, ...) that returns a new sim.

Maybe a better pattern for James' case: each file with a @defcomp defines a new Simulation instance and the main program merges them.

Copy link
Collaborator

@lrennels lrennels left a comment

Choose a reason for hiding this comment

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

Looks like some tests are failing so I will look into that!

@lrennels
Copy link
Collaborator

Looks good to me!

@lrennels lrennels merged commit 32aaa3b into master Jun 11, 2019
@lrennels lrennels deleted the empty-sim-ctor branch June 11, 2019 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants