Allow PROPERTIES to set a default value, and Modules to auto-initialise their DF columns to these values
#1436
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…set of Categories, and add class typehints to Property and Module
matt-graham
requested changes
Sep 6, 2024
There was a problem hiding this comment.
Thanks @willGraham01. Looks all sensible to me - have made a few minor suggestions regarding some of the type hints added, and one suggestion of adding a comment to clarify something that I initially misunderstood.
|
Checks are failing due to some syntax in files that are not affected by this PR. Review comments are addressed so happy to either wait for this to be fixed, or merge in now. |
Yeah this should be fixed by #1459
Shouldn't matter either way as we're already failing on |
matt-graham
approved these changes
Sep 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Concerns #1428 | Cherry-pick of the global functionality that was proposed in #1429
Introduces the functionality needed for us to remove a lot of repeated code in the
initialise_populationmethod forModules.Propertys can now have a default value set when they are created (falling back on the currently-provided defaults inPANDAS_TYPE_DEFAULT_TYPE_MAPif not provided) which will be used to fill the corresponding column in the population dataframe duringinitialise_population.Modules that just want to set their own property columns ininitialise_populationcan then directly inherit this implementation without needing to explicitly implement it themselves. IfModules want to run additional steps in this method, they can callsuper().initialise_populationfirst, and then provide their specific instructions afterwards. And finally, if aModulewants to do something completely different at this stage, they can just overwrite the method as normal and avoid callingsuper().As @tamuri suggested, we'll introduce the changes proposed in #1429 incrementally. This does mean we'll be supporting both methods of setting the property columns in the dataframe for a while, but gives us some granularity for bug-fixing/tracing.