Opened 5 years ago
Closed 5 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, GitHub, GitLab) | 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 5 years ago by
- Branch set to u/jhpalmieri/true-false
comment:2 Changed 5 years ago by
- Commit set to 864a42e11cbbca378eba2263fc88281ffb73f768
- Status changed from new to needs_review
comment:3 Changed 5 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 follow-up: ↓ 6 Changed 5 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 5 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 5 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 5 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 5 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/true-false
|
comment:10 Changed 5 years ago by
- Branch changed from u/jhpalmieri/true-false to u/jdemeyer/true-false
comment:11 Changed 5 years ago by
- Commit changed from 321e47bcf649d830eaab18534bdc2d70d42f2de2 to cf2da7bf84bac9c3c671fb8abf6269e911473208
comment:12 Changed 5 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 5 years ago by
- Commit changed from 75d391ccdde3d5e9465b4c7c5479ee5bf2b1aaba to 361223866e0bee9bfc19aebdaa04fd16ecc182f7
comment:14 follow-up: ↓ 15 Changed 5 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[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:
comment:15 in reply to: ↑ 14 Changed 5 years ago by
- Status changed from needs_work to needs_review
This ticket is not claiming to fix everything.
comment:16 Changed 5 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 5 years ago by
I'm fine with anything which does not make it harder for this ticket to get positive_review.
comment:18 Changed 5 years ago by
Then i will open a new ticket and continue the review ( I still have some tests running).
comment:19 Changed 5 years ago by
- Status changed from needs_review to positive_review
It's all good to me. Tests are ok.
comment:20 Changed 5 years ago by
- Reviewers set to Vincent Klein
comment:21 Changed 5 years ago by
- Branch changed from u/jdemeyer/true-false 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.