Opened 2 years ago
Closed 2 years ago
#31196 closed enhancement (fixed)
Code Improvements for Mutability module
Reported by: | gh-mjungmath | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.3 |
Component: | misc | Keywords: | |
Cc: | mkoeppe, tscrim | Merged in: | |
Authors: | Michael Jung | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | b9f3c28 (Commits, GitHub, GitLab) | Commit: | b9f3c2837df1fb8d72221e855c87de75058d548e |
Dependencies: | #31181 | Stopgaps: |
Description (last modified by )
I suggest some code improvements for the documentation (adding a note that an attribute à la _is_immutable
is required to check immutability), an additional method _require_immutable
(to require immutability in cases like hash functions) and Python 3 compatibility (using format
instead of %
as recommended in Python's doc).
Change History (41)
comment:1 Changed 2 years ago by
Branch: | → u/gh-mjungmath/clean_mutibility_code |
---|
comment:2 Changed 2 years ago by
Commit: | → aebc3f4fb9e0be8cbfc9fe01b5d94ae8c4a92fb7 |
---|---|
Description: | modified (diff) |
comment:3 Changed 2 years ago by
Status: | new → needs_review |
---|
comment:4 Changed 2 years ago by
Description: | modified (diff) |
---|
comment:5 Changed 2 years ago by
Description: | modified (diff) |
---|
comment:6 Changed 2 years ago by
Commit: | aebc3f4fb9e0be8cbfc9fe01b5d94ae8c4a92fb7 → cc8f76b5bec041f17c26d4798fd462f03ef9d9c2 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
cc8f76b | Trac #31196: code improvements
|
comment:7 Changed 2 years ago by
Commit: | cc8f76b5bec041f17c26d4798fd462f03ef9d9c2 → 15720856853249d192eb56192ff589d3f88b1892 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
1572085 | Trac #31196: if logic error
|
comment:8 Changed 2 years ago by
Alternatively, one could ask for the method is_immutable()
instead of the attribute? Then it wouldn't matter what happens behind the scenes, that's most flexible.
comment:9 follow-up: 10 Changed 2 years ago by
I strongly think the attribute name should be standardized and not look for the choice between the two. This will not only be faster but easier to maintain.
comment:10 Changed 2 years ago by
Replying to tscrim:
I strongly think the attribute name should be standardized and not look for the choice between the two. This will not only be faster but easier to maintain.
That makes sense. What about this instead:
Alternatively, one could ask for the method is_immutable() instead of the attribute?
Then, the method is standardized, which I would favorize.
And what do you think about the change of error message?
comment:11 follow-up: 12 Changed 2 years ago by
Commit: | 15720856853249d192eb56192ff589d3f88b1892 → df9a54cd7cef639c2875487635706317979b3304 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
df9a54c | Trac #31196: use "is_immutable()" + documentation
|
comment:12 Changed 2 years ago by
Replying to git:
Branch pushed to git repo; I updated commit sha1. New commits:
df9a54c Trac #31196: use "is_immutable()" + documentation
I had something like this in mind...
Should be even faster since getattr
is completely avoided. Moreover, the error message is now much clearer if the object doesn't support mutability.
comment:13 Changed 2 years ago by
Commit: | df9a54cd7cef639c2875487635706317979b3304 → 06b40bfcd0451da94249b73f46cdd29a70c7cb88 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
06b40bf | Trac #31196: comment indirect doctest
|
comment:14 Changed 2 years ago by
Dependencies: | → #31181 |
---|
comment:15 Changed 2 years ago by
Description: | modified (diff) |
---|
comment:16 follow-up: 17 Changed 2 years ago by
The problem with this approach is that it will raise an attribute error if it is used on a class that does not implement an is_immutable()
method. Although I mostly agree that it generally should if it is using this feature, but I could conceive of a reason why it wouldn't. I would say it is more likely that things will have some attribute to hold the mutability status than just a method to check it. Ultimately I would leave the check the way it currently is with somethign in the docstring stating that it is a requirement to use the decorator.
I am not 100% convinced the error message is more clear, but I am okay with it (once you remove the trailing period).
comment:17 follow-up: 19 Changed 2 years ago by
Replying to tscrim:
The problem with this approach is that it will raise an attribute error if it is used on a class that does not implement an
is_immutable()
method. Although I mostly agree that it generally should if it is using this feature, but I could conceive of a reason why it wouldn't. I would say it is more likely that things will have some attribute to hold the mutability status than just a method to check it. Ultimately I would leave the check the way it currently is with somethign in the docstring stating that it is a requirement to use the decorator.
I don't get your point entirely. Is your AttributeError
concern related only to the method or generally speaking? If it's related, I'd say we use the attribute only (even though I personally like methods more, because they are not interested in implementation details), drop the getattr
(because it's faster) and raise an attribute error if that is not implemented. I think it should definitely raise an attribute error in cases where we cannot say whether the object is immutable or not. The decorator wouldn't make much sense in situations like this, same for the error message.
I am not 100% convinced the error message is more clear, but I am okay with it (once you remove the trailing period).
Any ideas for a compromise? I think, from a developer's viewpoint, the original error message was more clear. But my plan, so far, is that this decorator should be used for the mutability checks in the manifold setting (after the pickling in #31182 had been solved).
comment:18 Changed 2 years ago by
For the records, I suggested this change because of #31194, to avoid possible incompatibilites in case of slight variations in the implementation.
comment:19 follow-up: 20 Changed 2 years ago by
Replying to gh-mjungmath:
Replying to tscrim:
The problem with this approach is that it will raise an attribute error if it is used on a class that does not implement an
is_immutable()
method. Although I mostly agree that it generally should if it is using this feature, but I could conceive of a reason why it wouldn't. I would say it is more likely that things will have some attribute to hold the mutability status than just a method to check it. Ultimately I would leave the check the way it currently is with something in the docstring stating that it is a requirement to use the decorator.I don't get your point entirely. Is your
AttributeError
concern related only to the method or generally speaking? If it's related, I'd say we use the attribute only (even though I personally like methods more, because they are not interested in implementation details), drop thegetattr
(because it's faster) and raise an attribute error if that is not implemented. I think it should definitely raise an attribute error in cases where we cannot say whether the object is immutable or not. The decorator wouldn't make much sense in situations like this, same for the error message.
I don't understand this distinction you're trying to make. If you use this decorator with a method on a class that does not implement an is_immutable
method, then you get an AttributeError
. However, it is potentially reasonable for a class to add an attribute _is_immutable
when it becomes immutable. I agree with your point on not depending on implementation details, but because we are trying to set the API for implementations, I feel like that is somewhat moot. There is also something to be said for having different error messages, but having more flexibility I think trumps this, although just barely.
I am not 100% convinced the error message is more clear, but I am okay with it (once you remove the trailing period).
Any ideas for a compromise? I think, from a developer's viewpoint, the original error message was more clear. But my plan, so far, is that this decorator should be used for the mutability checks in the manifold setting (after the pickling in #31182 had been solved).
I would generally avoid the decorator and just directly call stuff. It is nearly the same amount of code. While it does make it slightly easier to refactor, it obfuscates the code mode and slows it down. This is good for the more casual user, but for production code, I would avoid the decorator in this case.
comment:20 Changed 2 years ago by
Replying to tscrim:
...
Alright then. I thought it might be convenient. Too much thinking, eh? I'll just add a warning to make clear that such objects require a _is_immutable
attribute.
As for the manifold code, I should switch to _require_immutable
in Mutability
instead?
comment:21 Changed 2 years ago by
Commit: | 06b40bfcd0451da94249b73f46cdd29a70c7cb88 → 4c3393509d75c6a040b312732a1801f190b88142 |
---|
comment:23 Changed 2 years ago by
Description: | modified (diff) |
---|
comment:24 follow-ups: 25 29 Changed 2 years ago by
Replying to gh-mjungmath:
Like this?
I think this is good overall. However I have one question and comment:
Why is the _require_immutable
not a cpdef
method but instead a Python and cdef
? I think you need an except -1
added too.
comment:25 follow-up: 26 Changed 2 years ago by
Replying to tscrim:
I think this is good overall. However I have one question and comment:
Why is the
_require_immutable
not acpdef
method but instead a Python andcdef
?
I actually don't know. That was the setup I found there. Actually, due to #31182, I think it is better to make this class completely Pythonic. If there is a need for a Cythonic version, I suggest to introduce a SageObjectWithMutability
. See my comment.
I think you need an
except -1
too.
What do you mean?
comment:26 Changed 2 years ago by
Replying to gh-mjungmath:
Replying to tscrim:
I think this is good overall. However I have one question and comment:
Why is the
_require_immutable
not acpdef
method but instead a Python andcdef
?I actually don't know. That was the setup I found there. Actually, due to #31182, I think it is better to make this class completely Pythonic. If there is a need for a Cythonic version, I suggest to introduce a
SageObjectWithMutability
. See my comment.
-1 on this. I will comment on #31182 for more about this.
I think you need an
except -1
too.What do you mean?
https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#error-return-values
comment:27 Changed 2 years ago by
Commit: | 4c3393509d75c6a040b312732a1801f190b88142 → c805199394721b8957e88bebc79bffd9f319c856 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c805199 | Trac #31196: minor code improvements, py3 compatibility, documentation improved
|
comment:28 Changed 2 years ago by
Commit: | c805199394721b8957e88bebc79bffd9f319c856 → 65110905c2f11bca8eedacffbca0c9b32786147e |
---|
comment:29 follow-up: 30 Changed 2 years ago by
Replying to tscrim:
Replying to gh-mjungmath:
Like this?
I think this is good overall. However I have one question and comment:
Why is the
_require_immutable
not acpdef
method but instead a Python andcdef
? I think you need anexcept -1
added too.
Is that better? I think, the except *
syntax is needed since these functions do not return anything.
comment:30 Changed 2 years ago by
Replying to gh-mjungmath:
Replying to tscrim:
Replying to gh-mjungmath:
Like this?
I think this is good overall. However I have one question and comment:
Why is the
_require_immutable
not acpdef
method but instead a Python andcdef
? I think you need anexcept -1
added too.Is that better? I think, the
except *
syntax is needed since these functions do not return anything.
Built won't work: Exception clause not allowed for function returning Python object. You sure that the exception clause is needed here?
comment:31 Changed 2 years ago by
Commit: | 65110905c2f11bca8eedacffbca0c9b32786147e → e5228d3b87529167876c09b693f498e636677d49 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e5228d3 | Trac #31196: cpdef require methods + example added
|
comment:32 Changed 2 years ago by
Okay, I added an example that works perfectly and improved the documentation incidentally. The except clause is apparently not needed.
comment:33 Changed 2 years ago by
Commit: | e5228d3b87529167876c09b693f498e636677d49 → d957f73a5ac9ab8f198e9d6eb0a3cac266e5d8ba |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
d957f73 | Trac #31196: unnecessary line in docstring removed
|
comment:34 Changed 2 years ago by
Reviewers: | → Travis Scrimshaw |
---|
Ah, it's fine because it doesn't have a return value (and hence returns None
). Let's wait for the patchbot. If that is green, then positive review.
comment:35 follow-up: 37 Changed 2 years ago by
Also, while it is technically not your responsibility since it existed previously, it would be good add docstrings and tests to all methods, including cpdef
ones.
comment:36 Changed 2 years ago by
Commit: | d957f73a5ac9ab8f198e9d6eb0a3cac266e5d8ba → b9f3c2837df1fb8d72221e855c87de75058d548e |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
b9f3c28 | Trac #31196: coverage
|
comment:37 Changed 2 years ago by
Replying to tscrim:
Also, while it is technically not your responsibility since it existed previously, it would be good add docstrings and tests to all methods, including
cpdef
ones.
Done.
comment:41 Changed 2 years ago by
Branch: | u/gh-mjungmath/clean_mutibility_code → b9f3c2837df1fb8d72221e855c87de75058d548e |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
New commits:
Trac #31194: rename attribute
Trac #31196: code improvements