Skip to content
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

Dynamic inclusion of node types #60

Merged
merged 3 commits into from
Feb 19, 2024
Merged

Dynamic inclusion of node types #60

merged 3 commits into from
Feb 19, 2024

Conversation

dalonsoa
Copy link
Collaborator

In the current implementation, Model.nodes_types is populated when you create a Model object based of the models that are known to WSIMOD at that time, and regardless of whether those nodes are needed or not for the simulation. As a result:

  • Adding custom nodes, as done in Enable customisation #54 becomes more complicated as this needs to be modified manually.
  • Some parts of the code are run that are might not be needed, as there are no nodes of that type.

This PR takes a few steps to make this more flexible.

@barneydobson
Copy link
Collaborator

This seems sensible. As part of this would it be sensible to stop using the class.name as part of the model behaviour and instead make that a default node attribute? I had made this poor decision super early on and since realising that it's not a good idea it has now become all over the place. Maybe it is a separate issue because it seems to me a reasonably large task and the documentation here would need to be updated.

@barneydobson
Copy link
Collaborator

OK and further I have tried this branch on one of our commonly used models and am getting an error that I assume is related to the new registry:
image

I haven't tested the CLI yet so maybe I am using it wrong (although it works fine on the main branch, you might see some error warnings but these are negligible) - hopefully you can reproduce with below.
model.zip

@dalonsoa
Copy link
Collaborator Author

Ok, I've updated the code - it was just a typo in one place - and now the model works beginning to end without errors. Could you check if the actual numbers produced are still correct?

About your comment, yes let's make a separate issue.

@barneydobson
Copy link
Collaborator

I'll test it out on one of our terribly complicated models to see if results are the same

Copy link
Collaborator

@barneydobson barneydobson left a comment

Choose a reason for hiding this comment

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

I have tested for the Lea model and results are identical. I think that's sufficient for me!

@dalonsoa dalonsoa merged commit e3b66fe into main Feb 19, 2024
21 checks passed
@dalonsoa dalonsoa deleted the node_registration branch February 19, 2024 13:13
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.

None yet

2 participants