Opened 3 years ago

Closed 2 years ago

#22889 closed enhancement (fixed)

Replace 'if x != False' with 'if x', etc.

Reported by: jhpalmieri Owned by:
Priority: major Milestone: sage-8.0
Component: misc Keywords:
Cc: Merged in:
Authors: John Palmieri Reviewers: Vincent Klein
Report Upstream: N/A Work issues:
Branch: 3612238 (Commits) Commit: 361223866e0bee9bfc19aebdaa04fd16ecc182f7
Dependencies: Stopgaps:

Description

There are many instances in the Sage library of code like

if A == True:
...
if B == False:
...
if C == None:
...

which (most of the time) should be changed to

if A:
...
if not B:
...
if C is None:
...

There are some exceptions, for example if B can be True, False, or None, then we may want to use if B is False to distinguish between False and None. But most should be changed.

Change History (21)

comment:1 Changed 3 years ago by jhpalmieri

  • Branch set to u/jhpalmieri/true-false

comment:2 Changed 3 years ago by jhpalmieri

  • Commit set to 864a42e11cbbca378eba2263fc88281ffb73f768
  • Status changed from new to needs_review

New commits:

864a42etrac 22889: replace 'if x == True' with 'if x', etc.

comment:3 Changed 3 years ago by jdemeyer

I'm not entirely convinced with the != cases. My gut feeling is that x != False should be translated as x is not False. If x can only be True or False, why write x != False instead of the simpler x == True?

What do you think?

comment:4 follow-up: Changed 3 years ago by jhpalmieri

As to why write x != False instead of x == True, sometimes there are several possible values for x, for example as in plot/plot.py -- see below. So I can understand it, but in those cases, if x != False is equivalent to if x. I think the only situation where if x != False should be if x is not False is if x might be None or [], for example, and you want to treat those differently from when x is False. I tried to make sure that wasn't the situation in any of the changes I made.

  • in ode.pyx: this file is badly documented, but I think the changes here are okay.
  • in linear_code.py, if ans != False: as far as I can tell, ans can be either False or a list of positive length, so if ans != False: is the same as if ans. (ans is set to B1.is_isomorphic(B2), where is_isomorphic comes from sage.groups.perm_gps.partn_ref.refinement_binary. That method is not well documented: what are the possible return values? From the code, it should be either False or a list, as I said. Or can the list have length 0 somehow?)
  • in hyperplane_arrangement/plot.py, if hyperplane_legend != False: hyperplane_legend can be a bool or either of the strings "long" or "short", so the change is safe, just as in the previous case.
  • assert self._regular!=False: I already changed this to assert self._regular is not False
  • in plot/plot.py: detect_poles != False. detect_poles can be True, False or "show". So safe to change.

comment:5 Changed 3 years ago by jdemeyer

  • Component changed from PLEASE CHANGE to misc
  • Status changed from needs_review to needs_work

In any case, needs to be rebased.

comment:6 in reply to: ↑ 4 Changed 3 years ago by jdemeyer

Replying to jhpalmieri:

  • in linear_code.py, if ans != False: as far as I can tell, ans can be either False or a list of positive length, so if ans != False: is the same as if ans. (ans is set to B1.is_isomorphic(B2), where is_isomorphic comes from sage.groups.perm_gps.partn_ref.refinement_binary. That method is not well documented: what are the possible return values? From the code, it should be either False or a list, as I said. Or can the list have length 0 somehow?)

You don't seem very convinced, so I would prefer to use is False here.

comment:7 Changed 3 years ago by jhpalmieri

I just checked: the NonlinearBinaryCodeStruct used in linear_code.py will not produce a length zero list. The length of the list is self.degree, and self.degree is the number of columns in the matrix used to define self. If I plug in a matrix with zero columns, I get this lovely thing:

sage: NonlinearBinaryCodeStruct(matrix(GF(2), 2, 0))
python(69054,0x7fff7dcbe000) malloc: *** error for object 0x1afbd0002: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
------------------------------------------------------------------------
0   signals.so                          0x00000001113747f5 print_backtrace + 37
------------------------------------------------------------------------
Unhandled SIGABRT: An abort() occurred.
This probably occurred because a *compiled* module has a bug
in it and is not properly wrapped with sig_on(), sig_off().
Python will now terminate.
------------------------------------------------------------------------

So the list will always have a positive number of entries. I can still make the change to is False if you want; I don't care that much.

comment:8 Changed 3 years ago by git

  • Commit changed from 864a42e11cbbca378eba2263fc88281ffb73f768 to 321e47bcf649d830eaab18534bdc2d70d42f2de2

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

321e47bMerge branch 'develop' into t/22889/true-false

comment:9 Changed 3 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

Rebased.

comment:10 Changed 2 years ago by jdemeyer

  • Branch changed from u/jhpalmieri/true-false to u/jdemeyer/true-false

comment:11 Changed 2 years ago by jdemeyer

  • Commit changed from 321e47bcf649d830eaab18534bdc2d70d42f2de2 to cf2da7bf84bac9c3c671fb8abf6269e911473208

Rebased again and changed linear_code.py.


New commits:

96f09edMerge tag '8.0.beta12' into t/22889/true-false
cf2da7bUse "ans is False" instead of "not ans"

comment:12 Changed 2 years ago by git

  • Commit changed from cf2da7bf84bac9c3c671fb8abf6269e911473208 to 75d391ccdde3d5e9465b4c7c5479ee5bf2b1aaba

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

75d391cCheck "is False" from result of _indefinite_factorization()

comment:13 Changed 2 years ago by git

  • Commit changed from 75d391ccdde3d5e9465b4c7c5479ee5bf2b1aaba to 361223866e0bee9bfc19aebdaa04fd16ecc182f7

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

80e5ff2Check "is False" from result of _indefinite_factorization()
3612238Use "ans is not False" instead of "ans" for result of is_isomorphic()

comment:14 follow-up: Changed 2 years ago by vklein

  • Status changed from needs_review to needs_work

Some other instances can be changed

Targets
    Occurrences of 'if .* == None' in Directory sage/src/sage
Found Occurrences  (1 usage found)
    sage/schemes/riemann_surfaces  (1 usage found)
        riemann_surface.py  (1 usage found)
            1319if (self._differentials == None):
Targets
    Occurrences of 'if .* == True' in Directory sage/src/sage
Found Occurrences  (3 usages found)
    sage/calculus  (1 usage found)
        calculus.py  (1 usage found)
            866if hold == True:
    sage/interfaces  (1 usage found)
        phc.py  (1 usage found)
            85if output_list[i-3].count('success') > 0 or get_failures == True:
    sage/schemes/product_projective  (1 usage found)
        wehlerK3.py  (1 usage found)
            987if check == True:
Targets
    Occurrences of 'if .*==True' in Directory sage/src/sage
Found Occurrences  (2 usages found)
    sage/coding  (1 usage found)
        code_bounds.py  (1 usage found)
            217if field_based==True and (not is_prime_power(q)):
    sage/matroids  (1 usage found)
        matroid.pyx  (1 usage found)
            7718if matroids_plot_helpers.posdict_is_sane(self,pos_dict) ==True:

Targets
    Occurrences of 'if .*==False' in Directory sage/src/sage
Found Occurrences  (1 usage found)
    sage/schemes/affine  (1 usage found)
        affine_morphism.py  (1 usage found)
            678if is_AffineSpace(self.domain())==False:
Last edited 2 years ago by vklein (previous) (diff)

comment:15 in reply to: ↑ 14 Changed 2 years ago by jdemeyer

  • Status changed from needs_work to needs_review

This ticket is not claiming to fix everything.

comment:16 Changed 2 years ago by vklein

Ok. But is it Ok to fix the ones I listed in this ticket? Or should we open another one?

comment:17 Changed 2 years ago by jdemeyer

I'm fine with anything which does not make it harder for this ticket to get positive_review.

comment:18 Changed 2 years ago by vklein

Then i will open a new ticket and continue the review ( I still have some tests running).

comment:19 Changed 2 years ago by vklein

  • Status changed from needs_review to positive_review

It's all good to me. Tests are ok.

comment:20 Changed 2 years ago by vklein

  • Reviewers set to Vincent Klein

comment:21 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/true-false to 361223866e0bee9bfc19aebdaa04fd16ecc182f7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.