replace is_X functions / refactoring process

[Ralf Stephan]

Hi,

I stumbled into misunderstanding the is_Set function from sets/set.py, thinking without looking that it determines the set property of a class. But it just does isinstance(X, Set_generic), so why not write that?

In https://trac.sagemath.org/ticket/24443 it emerges there are several such useless global functions and I propose to replace their usage with the code they contain. For example is_Ring(X) would be just X in Rings() so you can see that they are even used inconsistently (compare is_Set).

I'm told that devs need to agree for this refactoring so here it goes, please voice your opinion, you can see it's bad coding and prone to be misunderstood.

Now something new---if you agree with the above, I would like to experiment with a new procedure for refactoring tickets that aims to make reviewing them much easier, which will be needed for larger refactorings that touch a lot of files. My idea is to write script(s) that change the code, mostly using the rope library. The reviewer would then review the script, run it on a clean branch and compare if the changes match. If it can be made to work, inevitable merge conflicts could be dealt with by applying the scripts again (on the changed develop for example). I see automated refactoring as the only way to deal with large refactorings in Sage.

Please add your ideas or opinions on this, thanks in advance.

PS:

https://github.com/python-rope/rope

One problem is that rope cannot do Cython, so we're stuck with Python-only experiments at the moment.

PPS:

pycharm cannot be scripted I believe, and I cannot fix their bugs, it's closed source

[Vincent Delecroix]

Hi Ralf,

You asked two independent questions in one e-mail which is a bad
strategy to get answers. Let me focus on "is_X" in this sub-thread.

To my mind, is_X(Z) is very unexplicit as it may be one of

1. isinstance(Z, X_class)

2. Z in X_category()

which are two different things. Note that sage.rings.ring.is_Ring is
actually doing a transition from 1 to 2 and it might be subtle to remove
it currently.

I am in favor of removing all is_X, but most of them needs to be done
case by case (mostly because of remaining "old parents"). Note that I
proposed to remove is_RealField/is_RealNumber and
is_ComplexField/is_ComplexNumber in respectively [#24457] and [#24489].

[#24457] https://trac.sagemath.org/ticket/24457
[#24489] https://trac.sagemath.org/ticket/24489

Cheers
Vincent

[Jeroen Demeyer]

On 2018-01-10 08:44, Ralf Stephan wrote:
> PS:
> https://github.com/python-rope/rope
> One problem is that rope cannot do Cython

Needless to say, that makes it absolutely unsuitable for Sage. Don't
waste your time with it.

[Travis Scrimshaw]

To my mind, is_X(Z) is very unexplicit as it may be one of

  1. isinstance(Z, X_class)

  2. Z in X_category()

which are two different things. Note that sage.rings.ring.is_Ring is
actually doing a transition from 1 to 2 and it might be subtle to remove
it currently.

I am in favor of removing all is_X, but most of them needs to be done
case by case (mostly because of remaining "old parents").

+1 for removing the is_X methods when they are trivial, e.g., just an isinstance check. However, there are some non-trivial is_X functions IIRC. So I am not convinced we should remove those.

Best,
Travis

[Nicolas M. Thiery]

On Wed, Jan 10, 2018 at 10:50:04AM -0800, Travis Scrimshaw wrote:
> +1 for removing the is_X methods when they are trivial, e.g., just an
> isinstance check. However, there are some non-trivial is_X functions
> IIRC. So I am not convinced we should remove those.

+1 to Vincent and Travis's answers.

Just a complement: there is a semantic different between X in Foos()
-- is X already known to be a Foo -- and X.is_foo() -- really test if
X is a Foo, possibly running costly calculations if needed. So we need
to keep both. On the other hand, I would assume that is_Foo(X) is the
same as X.is_foo() in most cases; in that case, the former can safely
be deprecated, and we should do it. We may want to use the occasion to
switch some of the calls to X in Foos(), when it's more about "type
checking" than asking a math question.

Cheers,
Nicolas
--
Nicolas M. Thiéry "Isil" <nth...@users.sf.net>
http://Nicolas.Thiery.name/

[Kwankyu Lee]

The rationale of removing "is_X(Z)" should be for consistency, not that  it is implemented simply as "isinstance(Z, X_type)". I would assume "is_X(Z)" are mostly math questions, either simple or hard. I don't want to use "isinstance" to ask a math question.

So +1 for moving toward deprecating them in favor of "Z in Xs()", for consistency. Then "Z in Xs()" could be implemented either as "isinstance(Z, X_type)", "Z.is_x()", or something.