Code Improvements for Mutability module

[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).

[gh-mjungmath]

New commits:

​2e918ad	Trac #31194: rename attribute
​aebc3f4	Trac #31196: code improvements

[git]

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

​cc8f76b

Trac #31196: code improvements

[git]

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

​1572085

Trac #31196: if logic error

[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.

[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.

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

[git]

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

​df9a54c

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

[gh-mjungmath]

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.

[git]

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

​06b40bf

Trac #31196: comment indirect doctest

[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).

[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).

[gh-mjungmath]

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

[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.

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

[git]

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

​5ecbf3a	Trac #31181: return added
​4c33935	Trac #31196: minor code improvements, py3 compatibility, documentation improved

[gh-mjungmath]

Like this?

[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.

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

[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

[git]

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

[git]

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

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

[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.

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

[git]

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

​e5228d3

Trac #31196: cpdef require methods + example added

[gh-mjungmath]

Okay, I added an example that works perfectly and improved the documentation incidentally. The except clause is apparently not needed.

[git]

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

​d957f73

Trac #31196: unnecessary line in docstring removed

[tscrim]

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.

[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.

[git]

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

​b9f3c28

Trac #31196: coverage

[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.

[gh-mjungmath]

Patchbot is satisfied. :)

[tscrim]

Thank you. LGTM.

[gh-mjungmath]

Thanks for the review!