Improve argument singletons #1760
Conversation
|
Hello again! Thank you for this new pull request 🤩. Please begin by requesting your checklist using the command |
|
Here is your checklist. Please tick items off when you have completed them or determined that they are not necessary for this pull request:
|
|
Unfortunately your PR is not passing the tests so it is not quite ready for review yet. Let me know when it is fixed with |
![pyccel-bot[bot]](https://avatars.githubusercontent.com/in/364561?s=60&v=4){kind=small}
|
Hey @pyccel/pyccel-dev ! @EmilyBourne has just created this great new pull request! Check it out and let me know what you think! |
pyccel-bot
bot
added
Ready_for_review
|
Hey @yguclu, @EmilyBourne, this PR is looking pretty good. @EmilyBourne and @mustapha-belbiad think it is ready to merge. Could you add your expertise to confirm that this follows all the coding conventions and fits in Pyccel's future plans? Thanks 😄 |
EmilyBourne
removed
the
Ready_for_review
EmilyBourne
dismissed
mustapha-belbiad’s stale review
Rewrite coming. Review will no longer be relevant
|
Hey @pyccel/pyccel-dev ! @EmilyBourne has just created this great new pull request! Check it out and let me know what you think! |
pyccel/utilities/metaclasses.py
Outdated
| bases : tuple[class,...] | ||
| A tuple of the superclasses of the class. | ||
| dct : dict | ||
| A dictionary of the class attributes. | ||
| """ | ||
| _instances = {} |
There was a problem hiding this comment.
I find it a bit confusing to give an _instances attribute to the metaclass. This will mean that the dictionary is in the metaclass rather than in each of the actual argument-singleton types. Why don't we create the dictionary in the __init__ instead?
class ArgumentSingleton(type):
def __init__(cls, name, bases, dct):
cls._instances = {} # Dictionary local to each argument-singleton type
cls.__signature__ = signature(cls.__init__)
super().__init__(name, bases, dct)There was a problem hiding this comment.
I agree. I just kept the old implementation without thinking about it. Fixed: 4c0369d
There was a problem hiding this comment.
If the dictionary is local to each argument-singleton type, it sounds like there is no need to include cls in the dictionary key. What do you think?
There was a problem hiding this comment.
If my last comment is correct, then we have no need to use a dictionary in the "simple" singleton case. And in fact it would be beneficial not to have the metaclass Singleton subclass ArgumentSingleton, in order to simplify the code. E.g. see this code.
There was a problem hiding this comment.
I agree. I have removed it: b273ef9
The original reason for making Singleton subclass ArgumentSingleton was to allow a class (DataType) to have the general metaclass (ArgumentSingleton), and the subclasses to redefine it (which is only allowed if the new metaclass is a subclass). But most of the time we want Singleton anyway and I am not sure that this is really helpful. I have split them back up again. 3405e0f
|
/bot run pr_tests |
|
Hey @pyccel/pyccel-dev ! @EmilyBourne has just created this great new pull request! Check it out and let me know what you think! |
|
Hey @pyccel/pyccel-dev ! @EmilyBourne has just created this great new pull request! Check it out and let me know what you think! |
pyccel-bot
bot
added
Ready_to_merge
The class
ArgumentSingletoncurrently lets us create singleton classes such that there is only one instance of the class for any given set of arguments. However our current implementation relies on*args, **kwargsto guarantee compatibility with the__init__function of the class. This results infunctools.signaturereporting the wrong signature. As a result numpydoc insists on docstrings matching the prototype ofArgumentSingleton.__call__. This is manageable when there are few classes using the metaclass (as is the case currently), but it is unhelpful when the class is used more widely (as is the case in #1756). The solution to this is to use the__signature__property documented ininspect.signatureto trick numpydoc into seeing the signature of the__init__function.