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:

Status badges

Description (last modified by gh-mjungmath)

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 gh-mjungmath

Branch: u/gh-mjungmath/clean_mutibility_code

comment:2 Changed 2 years ago by gh-mjungmath

Commit: aebc3f4fb9e0be8cbfc9fe01b5d94ae8c4a92fb7
Description: modified (diff)

New commits:

2e918adTrac #31194: rename attribute
aebc3f4Trac #31196: code improvements

comment:3 Changed 2 years ago by gh-mjungmath

Status: newneeds_review

comment:4 Changed 2 years ago by gh-mjungmath

Description: modified (diff)

comment:5 Changed 2 years ago by gh-mjungmath

Description: modified (diff)

comment:6 Changed 2 years ago by git

Commit: aebc3f4fb9e0be8cbfc9fe01b5d94ae8c4a92fb7cc8f76b5bec041f17c26d4798fd462f03ef9d9c2

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

cc8f76bTrac #31196: code improvements

comment:7 Changed 2 years ago by git

Commit: cc8f76b5bec041f17c26d4798fd462f03ef9d9c215720856853249d192eb56192ff589d3f88b1892

Branch pushed to git repo; I updated commit sha1. New commits:

1572085Trac #31196: if logic error

comment:8 Changed 2 years ago by gh-mjungmath

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 Changed 2 years ago by 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.

comment:10 in reply to:  9 Changed 2 years ago by gh-mjungmath

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?

Last edited 2 years ago by gh-mjungmath (previous) (diff)

comment:11 Changed 2 years ago by git

Commit: 15720856853249d192eb56192ff589d3f88b1892df9a54cd7cef639c2875487635706317979b3304

Branch pushed to git repo; I updated commit sha1. New commits:

df9a54cTrac #31196: use "is_immutable()" + documentation

comment:12 in reply to:  11 Changed 2 years ago by gh-mjungmath

Replying to git:

Branch pushed to git repo; I updated commit sha1. New commits:

df9a54cTrac #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.

Last edited 2 years ago by gh-mjungmath (previous) (diff)

comment:13 Changed 2 years ago by git

Commit: df9a54cd7cef639c2875487635706317979b330406b40bfcd0451da94249b73f46cdd29a70c7cb88

Branch pushed to git repo; I updated commit sha1. New commits:

06b40bfTrac #31196: comment indirect doctest

comment:14 Changed 2 years ago by gh-mjungmath

Dependencies: #31181

comment:15 Changed 2 years ago by gh-mjungmath

Description: modified (diff)

comment:16 Changed 2 years ago by 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 am not 100% convinced the error message is more clear, but I am okay with it (once you remove the trailing period).

comment:17 in reply to:  16 ; Changed 2 years ago by 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 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).

Last edited 2 years ago by gh-mjungmath (previous) (diff)

comment:18 Changed 2 years ago by gh-mjungmath

For the records, I suggested this change because of #31194, to avoid possible incompatibilites in case of slight variations in the implementation.

comment:19 in reply to:  17 ; Changed 2 years ago by tscrim

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 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 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 in reply to:  19 Changed 2 years ago by gh-mjungmath

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?

Last edited 2 years ago by gh-mjungmath (previous) (diff)

comment:21 Changed 2 years ago by git

Commit: 06b40bfcd0451da94249b73f46cdd29a70c7cb884c3393509d75c6a040b312732a1801f190b88142

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5ecbf3aTrac #31181: return added
4c33935Trac #31196: minor code improvements, py3 compatibility, documentation improved

comment:22 Changed 2 years ago by gh-mjungmath

Description: modified (diff)

Like this?

comment:23 Changed 2 years ago by gh-mjungmath

Description: modified (diff)

comment:24 in reply to:  22 ; Changed 2 years ago by 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 a cpdef method but instead a Python and cdef? I think you need an except -1 added too.

comment:25 in reply to:  24 ; Changed 2 years ago by gh-mjungmath

Replying to tscrim:

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 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?

Last edited 2 years ago by gh-mjungmath (previous) (diff)

comment:26 in reply to:  25 Changed 2 years ago by tscrim

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 a cpdef method but instead a Python and cdef?

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 git

Commit: 4c3393509d75c6a040b312732a1801f190b88142c805199394721b8957e88bebc79bffd9f319c856

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c805199Trac #31196: minor code improvements, py3 compatibility, documentation improved

comment:28 Changed 2 years ago by git

Commit: c805199394721b8957e88bebc79bffd9f319c85665110905c2f11bca8eedacffbca0c9b32786147e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5ecbf3aTrac #31181: return added
4c33935Trac #31196: minor code improvements, py3 compatibility, documentation improved
6511090Trac #31196: cpdef require methods + except *

comment:29 in reply to:  24 ; Changed 2 years ago by 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 a cpdef method but instead a Python and cdef? I think you need an except -1 added too.

Is that better? I think, the except * syntax is needed since these functions do not return anything.

comment:30 in reply to:  29 Changed 2 years ago by gh-mjungmath

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 a cpdef method but instead a Python and cdef? I think you need an except -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 git

Commit: 65110905c2f11bca8eedacffbca0c9b32786147ee5228d3b87529167876c09b693f498e636677d49

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e5228d3Trac #31196: cpdef require methods + example added

comment:32 Changed 2 years ago by gh-mjungmath

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 git

Commit: e5228d3b87529167876c09b693f498e636677d49d957f73a5ac9ab8f198e9d6eb0a3cac266e5d8ba

Branch pushed to git repo; I updated commit sha1. New commits:

d957f73Trac #31196: unnecessary line in docstring removed

comment:34 Changed 2 years ago by tscrim

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.

Last edited 2 years ago by tscrim (previous) (diff)

comment:35 Changed 2 years ago by 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.

comment:36 Changed 2 years ago by git

Commit: d957f73a5ac9ab8f198e9d6eb0a3cac266e5d8bab9f3c2837df1fb8d72221e855c87de75058d548e

Branch pushed to git repo; I updated commit sha1. New commits:

b9f3c28Trac #31196: coverage

comment:37 in reply to:  35 Changed 2 years ago by gh-mjungmath

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:38 Changed 2 years ago by gh-mjungmath

Patchbot is satisfied. :)

comment:39 Changed 2 years ago by tscrim

Status: needs_reviewpositive_review

Thank you. LGTM.

comment:40 Changed 2 years ago by gh-mjungmath

Thanks for the review!

comment:41 Changed 2 years ago by vbraun

Branch: u/gh-mjungmath/clean_mutibility_codeb9f3c2837df1fb8d72221e855c87de75058d548e
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.