Abstract provides defaults #1528
Open
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
rowleya
reviewed
Jan 22, 2025
Comment on lines
+207
to
+208
| while hasattr(init, "_method"): | ||
| init = getattr(init, "_method") |
There was a problem hiding this comment.
Does this terminate? How does the attribute get removed?
There was a problem hiding this comment.
I assume it does terminate as it is the same code as before... sometimes my own code confuses me!
There was a problem hiding this comment.
Oh I see now, it is essentially recusing the init until it finds the real init
rowleya
approved these changes
Jan 22, 2025
I would probably say deprecate as otherwise people might have things broken at the last minute by accidentally updating... |
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.
In master there are two ways that the decorators @default_parameters and @default_initial_values are converted to a mapping of the values in the init. Making these properties
@defaults
This calculated the mappings when the class is imported (even if not used)
The resulting mapping replace the calculate functions so the calculation is only done once!
This style also did not have properties for mypy to find
AbstractPyNNModel
This calculated the mapping every time the property was called
Only worked for classes that extended AbstractPyNNModel
This PR merges the best of both solutions into AbstractProvidesDefaults
AbstractPyNNModel and any class which used to be decorated by @defaults now extends AbstractPyNNModel
The calculation of the mappings is delayed until either property is used.
Then BOTH properties are filled in (as the calculation uses a lot of sharable work)
Tested by:
SpiNNakerManchester/IntegrationTests#307
question.
Which should we do: