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:  sage8.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
 Branch set to u/jhpalmieri/truefalse
comment:2 Changed 3 years ago by
 Commit set to 864a42e11cbbca378eba2263fc88281ffb73f768
 Status changed from new to needs_review
comment:3 Changed 3 years ago by
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 followup: ↓ 6 Changed 3 years ago by
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 eitherFalse
or a list of positive length, soif ans != False:
is the same asif ans
. (ans
is set toB1.is_isomorphic(B2)
, whereis_isomorphic
comes fromsage.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 eitherFalse
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 toassert self._regular is not False
 in plot/plot.py:
detect_poles != False
.detect_poles
can beTrue
,False
or"show"
. So safe to change.
comment:5 Changed 3 years ago by
 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
Replying to jhpalmieri:
 in linear_code.py,
if ans != False
: as far as I can tell,ans
can be eitherFalse
or a list of positive length, soif ans != False:
is the same asif ans
. (ans
is set toB1.is_isomorphic(B2)
, whereis_isomorphic
comes fromsage.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 eitherFalse
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
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
 Commit changed from 864a42e11cbbca378eba2263fc88281ffb73f768 to 321e47bcf649d830eaab18534bdc2d70d42f2de2
Branch pushed to git repo; I updated commit sha1. New commits:
321e47b  Merge branch 'develop' into t/22889/truefalse

comment:10 Changed 2 years ago by
 Branch changed from u/jhpalmieri/truefalse to u/jdemeyer/truefalse
comment:11 Changed 2 years ago by
 Commit changed from 321e47bcf649d830eaab18534bdc2d70d42f2de2 to cf2da7bf84bac9c3c671fb8abf6269e911473208
comment:12 Changed 2 years ago by
 Commit changed from cf2da7bf84bac9c3c671fb8abf6269e911473208 to 75d391ccdde3d5e9465b4c7c5479ee5bf2b1aaba
Branch pushed to git repo; I updated commit sha1. New commits:
75d391c  Check "is False" from result of _indefinite_factorization()

comment:13 Changed 2 years ago by
 Commit changed from 75d391ccdde3d5e9465b4c7c5479ee5bf2b1aaba to 361223866e0bee9bfc19aebdaa04fd16ecc182f7
comment:14 followup: ↓ 15 Changed 2 years ago by
 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[i3].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:
comment:15 in reply to: ↑ 14 Changed 2 years ago by
 Status changed from needs_work to needs_review
This ticket is not claiming to fix everything.
comment:16 Changed 2 years ago by
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
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
Then i will open a new ticket and continue the review ( I still have some tests running).
comment:19 Changed 2 years ago by
 Status changed from needs_review to positive_review
It's all good to me. Tests are ok.
comment:20 Changed 2 years ago by
 Reviewers set to Vincent Klein
comment:21 Changed 2 years ago by
 Branch changed from u/jdemeyer/truefalse to 361223866e0bee9bfc19aebdaa04fd16ecc182f7
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac 22889: replace 'if x == True' with 'if x', etc.