Opened 3 years ago

Closed 3 years ago

#26141 closed defect (fixed)

fix bad code quality for recently merged #24837

Reported by: dkrenn Owned by:
Priority: minor Milestone: sage-8.4
Component: geometry Keywords:
Cc: jipilab, tscrim, moritz, vbraun, vdelecroix Merged in:
Authors: Jean-Philippe Labbé, Daniel Krenn Reviewers: Daniel Krenn, Jean-Philippe Labbé
Report Upstream: N/A Work issues:
Branch: 31765a5 (Commits, GitHub, GitLab) Commit: 31765a5b15e3f7611c55d184857376a9b2350491
Dependencies: #24837 Stopgaps:

Status badges

Description (last modified by tscrim)

In #24837 the method repr_pretty_Hrepresentation was changed to Hrepresentation_str but unfortunately the quality of the code does not meet Python's and SageMath's standards. The issues (relating to #24837, https://git.sagemath.org/sage.git/commit?id=c868d2c4a4e8b78a8221291741e878d468ddf367):

  1. There is no parameter setting which gives back the old default behavior, which is inequalities separated by commas. Using the separator, one only gets strings containing a lot of blanks, which simple cannot be read properly.
  2. The default values changed without a deprecation. The deprecation just aliases the old and new methods, but does not take care of the parameter setting. (I understand that some believe that this is a minor issue as the output is "only" a string represented to the user and not an (intermediate) result of a computation. However, one could have done this more smoothly)
  3. PEP8 is not satisfied, in particular some too long lines and a lot of missing blanks after commas.
  4. The code in some lines/blocks is considered bad in a Pythonic sense:
    shift = any([pretty_hs[index][2][0] == '-' for index in range(len(pretty_hs))])
    
    should be something like
    shift = any(pretty_h[2][0] == '-' for pretty_h in pretty_hs)
    
    or even better
    shift = any(pretty_h[2].startswith('-') for pretty_h in pretty_hs)
    
    Also get rid of the second for index in range(len(...))
  5. The line
    pretty_print = pretty_print[:-1] # Removing the last return
    
    only works if the separator has length 1. Hint: The Pythonic way to do this kind of concatentation is to use separator.join(...).
  6. I cannot think of any reason, why the output has an additional blank at each end of line. Please remove this. (I personally would also get rid of all the blanks at the end of the line, i.e. the padding on the right. This maybe is a matter of taste, so should not bother us too much and probably people will find a lot of arguments why this is something wanted, so I just comment this here. ;) )
  7. Why
    product = -1*(coeffs[1:]*vars[1:])
    
    and not simply
    product = -(...)
    
    or even
    product = -...
    

?

  1. There is quite a lot of code doubling at the end of the patch of #24837:
    if not split:
        some code
    else:
        almost identical code with very few exceptions
    
    This is very hard too read as one has to compare both almost identical blocks to find the differences. I am certain that a better solution can be found for this.
  2. Docstring of style-parameter: Shouldn't the commas be set differently: either ..., or ... or ... because the latter two should be grouped and not the former two?
  3. Docstring
    ``latex`` -- a boolean. Default is ``None``
    
    So what does None mean here? Is it set automatically according to the input? If static, probably its False but who knows. Why using None at all and not False as the default then?
  4. The default of the style-parameter is '>=' in base.py and 'positive' in representation.py. Having different default values within the same module for methods doing "the same" is usually not a good idea.
  5. The variable split needs a docstring explaining what it does as this is not obvious. (And, from a technical point of view, one could consider using its inverse join, as it is more a ' '.join(...) than a ....split(' '), but this is highly debatable.)

Change History (15)

comment:1 Changed 3 years ago by jdemeyer

I won't comment on this specific case, but you could also consider reverting #24837 and then fixing these issues. In any case, it would be good to have one of these 3 things soon:

  1. Agreement that the code is not so bad after all.
  1. A fix for the code.
  1. A revert of the code.
Last edited 3 years ago by jdemeyer (previous) (diff)

comment:2 follow-up: Changed 3 years ago by tscrim

  • Description modified (diff)
  • Priority changed from blocker to minor

Dear Daniel, none of these issues are blockers to me. Please don't throw such strong statements around carelessly. Also, since a lot of these issues are simple, why are you asking others to do it? Why can't you take care of them yourself? If you have time to review, you have time to code.

Also, code does not need to be perfect to get into Sage (something about the enemy of good comes to mind). I am happy to delegate work to subsequent tickets.

Some technical notes. PEP8 are guidelines, good ones that should be followed as much as possible, but guidelines all the same. Same for "Pythonic" code (also you can get a speed bonus for doing these things, not that it is particularly useful here). Sometimes you have to duplicate code or you have to make things somewhat convoluted to not do the duplication. Minor differences are differences. It is also really annoying to deprecate default value changes.

comment:3 in reply to: ↑ 2 Changed 3 years ago by dkrenn

Replying to tscrim:

Dear Daniel, none of these issues are blockers to me. Please don't throw such strong statements around carelessly. Also, since a lot of these issues are simple, why are you asking others to do it? Why can't you take care of them yourself? If you have time to review, you have time to code.

Also, code does not need to be perfect to get into Sage (something about the enemy of good comes to mind). I am happy to delegate work to subsequent tickets.

It is true that each of the issues above is not a blocker, but by finding that many issues in one simple patch one gets the impression that the code was written sloppy and that the review was done sloppy. And this is something we do not want (as SageMath is sometimes advertised with high coding standards and a peer-reviewing process); this is the actual issue here.

Some technical notes. PEP8 are guidelines, good ones that should be followed as much as possible, but guidelines all the same.

I wouldn't have brought PEP8 up alone (and I do not mind if there is a too long line from time to time), but the code itself is very inconsistently written, so this is way it came up.

Same for "Pythonic" code (also you can get a speed bonus for doing these things, not that it is particularly useful here).

I do not insist on this either, but as I have re-reviewed the ticket, I brought it up.

Sometimes you have to duplicate code or you have to make things somewhat convoluted to not do the duplication. Minor differences are differences.

In this particular case it is very easy to avoid the doubling: Use the complete code from the else-block but instead of return save the content in a variable. Then

if not split:
    return ' '.join(variable)
else:
    return variable

It is also really annoying to deprecate default value changes.

Yes, but the major issue here is that the original behavior is simply not possible anymore; this is annoying.

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

Hello,

Thank you very much for your 12 points-deep review of this recent ticket.

I am on it.

comment:5 in reply to: ↑ 4 Changed 3 years ago by dkrenn

Replying to jipilab:

Thank you very much for your 12 points-deep review of this recent ticket.

I am on it.

Great :) If you have any questions, don't hesitate to ask :)

comment:6 follow-up: Changed 3 years ago by jipilab

  • Authors set to Jean-Philippe Labbé
  • Branch set to u/jipilab/26141
  • Commit set to d7ec556f1ce336f2efcff32f77b7aa1ecb57a4cd
  1. There is no parameter setting which gives back the old default behavior, which is inequalities separated by commas. Using the separator, one only gets strings containing a lot of blanks, which simple cannot be read properly.
  2. The default values changed without a deprecation. The deprecation just aliases the old and new methods, but does not take care of the parameter setting. (I understand that some believe that this is a minor issue as the output is "only" a string represented to the user and not an (intermediate) result of a computation. However, one could have done this more smoothly)

I agree that changing the default behavior is a sensible issue, and that the transition could have been more smooth, see https://xkcd.com/1172 . The code now gives:

sage: c = polytopes.cube()
sage: c.repr_pretty_Hrepresentation(separator=', ',style='positive')
/home/jplabbe/sage/src/bin/sage-ipython:1: DeprecationWarning: repr_pretty_Hrepresentation is deprecated. Please use Hrepresentation_str instead.
See https://trac.sagemath.org/24837 for details.
  #!/usr/bin/env sage-python23
'1 >= x2, 1 >= x1, 1 >= x0, x0 + 1 >= 0, x2 + 1 >= 0, x1 + 1 >= 0'

so it is possible to get back the previous behavior.

  1. PEP8 is not satisfied, in particular some too long lines and a lot of missing blanks after commas.

The ticket #24837 introduced 10 too long lines and 12 missing spaces after commas. I added the necessary spaces after the commas in all the file and I left the other four hundred fifty E501s alone.

  1. The code in some lines/blocks is considered bad in a Pythonic sense:
    shift = any([pretty_hs[index][2][0] == '-' for index in range(len(pretty_hs))])
    
    should be something like
    shift = any(pretty_h[2][0] == '-' for pretty_h in pretty_hs)
    
    or even better
    shift = any(pretty_h[2].startswith('-') for pretty_h in pretty_hs)
    
    Also get rid of the second for index in range(len(...))

Thanks for mentioning, forgot to look over that.

  1. The line
    pretty_print = pretty_print[:-1] # Removing the last return
    
    only works if the separator has length 1. Hint: The Pythonic way to do this kind of concatentation is to use separator.join(...).

Done.

  1. I cannot think of any reason, why the output has an additional blank at each end of line. Please remove this. (I personally would also get rid of all the blanks at the end of the line, i.e. the padding on the right. This maybe is a matter of taste, so should not bother us too much and probably people will find a lot of arguments why this is something wanted, so I just comment this here. ;) )

The reason was the shifting of coefficients appearing on the right column. This column appearing on the right of the inequality sign is left justified. When a minus sign appears the coefficient start 1 character more to the left. When there is no minus sign before the coefficient, it therefore appears shifted towards the left. In other to deal with this alignment issue, we insert one character more to the right column to place the "-". This had the side effect that when no minus sign appears (which is the case when "style=positive") then, there is a space left at the end of an equation.

This is now taken care of.

  1. Why
    product = -1*(coeffs[1:]*vars[1:])
    
    and not simply
    product = -(...)
    
    or even
    product = -...
    

?

Done.

  1. There is quite a lot of code doubling at the end of the patch of #24837:
    if not split:
        some code
    else:
        almost identical code with very few exceptions
    
    This is very hard too read as one has to compare both almost identical blocks to find the differences. I am certain that a better solution can be found for this.

Done.

  1. Docstring of style-parameter: Shouldn't the commas be set differently: either ..., or ... or ... because the latter two should be grouped and not the former two?

Jedem Tierchen sein Pläsierchen. I do not argue on this.

  1. Docstring
    ``latex`` -- a boolean. Default is ``None``
    
    So what does None mean here? Is it set automatically according to the input? If static, probably its False but who knows. Why using None at all and not False as the default then?

Done.

  1. The default of the style-parameter is '>=' in base.py and 'positive' in representation.py. Having different default values within the same module for methods doing "the same" is usually not a good idea.

Forgot to change it. Thanks, they are now both '>='.

  1. The variable split needs a docstring explaining what it does as this is not obvious. (And, from a technical point of view, one could consider using its inverse join, as it is more a ' '.join(...) than a ....split(' '), but this is highly debatable.)

No opinion here.


New commits:

d7ec556Fix bad code quality of 24837

comment:7 Changed 3 years ago by jipilab

  • Status changed from new to needs_review

comment:8 Changed 3 years ago by dkrenn

  • Branch changed from u/jipilab/26141 to u/dkrenn/26141

comment:9 in reply to: ↑ 6 ; follow-up: Changed 3 years ago by dkrenn

  • Commit changed from d7ec556f1ce336f2efcff32f77b7aa1ecb57a4cd to e83534113d56ef5d80f3c02a517c8e3bee572e96

Replying to jipilab:

I agree that changing the default behavior is a sensible issue, and that the transition could have been more smooth, see https://xkcd.com/1172 .

I know :)))

The code now gives: [...] so it is possible to get back the previous behavior.

Great. Thanks.

  1. PEP8 is not satisfied, in particular some too long lines and a lot of missing blanks after commas.

The ticket #24837 introduced 10 too long lines and 12 missing spaces after commas. I added the necessary spaces after the commas in all the file and I left the other four hundred fifty E501s alone.

Thank you for doing all these changes. I've added two more space after a comma.

  1. The code in some lines/blocks is considered bad in a Pythonic sense:

[...]

or even better

shift = any(pretty_h[2].startswith('-') for pretty_h in pretty_hs)

Also get rid of the second for index in range(len(...))

Thanks for mentioning, forgot to look over that.

LGTM.

  1. The line
    pretty_print = pretty_print[:-1] # Removing the last return
    
    only works if the separator has length 1. Hint: The Pythonic way to do this kind of concatentation is to use separator.join(...).

Done.

LGTM. I did a minor rewrite to hopefully improve readablity.

  1. I cannot think of any reason, why the output has an additional blank at each end of line. Please remove this. (I personally would also get rid of all the blanks at the end of the line, i.e. the padding on the right. This maybe is a matter of taste, so should not bother us too much and probably people will find a lot of arguments why this is something wanted, so I just comment this here. ;) )

The reason was the shifting of coefficients appearing on the right column. This column appearing on the right of the inequality sign is left justified. When a minus sign appears the coefficient start 1 character more to the left. When there is no minus sign before the coefficient, it therefore appears shifted towards the left. In other to deal with this alignment issue, we insert one character more to the right column to place the "-". This had the side effect that when no minus sign appears (which is the case when "style=positive") then, there is a space left at the end of an equation.

This is now taken care of.

Thank you for the clearification and for taking care of.

  1. Why
    product = -1*(coeffs[1:]*vars[1:])
    
    and not simply
    product = -(...)
    
    or even
    product = -...
    

?

Done.

LGTM.

  1. There is quite a lot of code doubling at the end of the patch of #24837:
    if not split:
        some code
    else:
        almost identical code with very few exceptions
    
    This is very hard too read as one has to compare both almost identical blocks to find the differences. I am certain that a better solution can be found for this.

Done.

Much better now, thank you.

  1. Docstring of style-parameter: Shouldn't the commas be set differently: either ..., or ... or ... because the latter two should be grouped and not the former two?

Jedem Tierchen sein Pläsierchen. I do not argue on this.

I've changed this now. Please veto, if you it doesn't work for you.

  1. Docstring
    ``latex`` -- a boolean. Default is ``None``
    
    So what does None mean here? Is it set automatically according to the input? If static, probably its False but who knows. Why using None at all and not False as the default then?

Done.

Thanks.

  1. The default of the style-parameter is '>=' in base.py and 'positive' in representation.py. Having different default values within the same module for methods doing "the same" is usually not a good idea.

Forgot to change it. Thanks, they are now both '>='.

Ok. (FWIW, I would be happy with any of the two being the default.)

  1. The variable split needs a docstring explaining what it does as this is not obvious. (And, from a technical point of view, one could consider using its inverse join, as it is more a ' '.join(...) than a ....split(' '), but this is highly debatable.)

I've shortend the OUTPUT-block to avoid the double-description of the style-parameter.


New commits:

b83b52bTrac #26141: one PEP8 space after comma
9ecefa0Trac #26141: code simplification split/length
91fa86eTrac #26141: new parameter align
e447b5eTrac #26141: trying to improve readability even more
7967bcfTrac #26141: a PEP8 line break and space-comma
e52b936Trac #26141: align docstring and remove comma
e835341Trac #26141: rewrite OUTPUT block to avoid doubling information on style-parameter

comment:10 in reply to: ↑ 9 Changed 3 years ago by dkrenn

Replying to dkrenn:

9ecefa0Trac #26141: code simplification split/length

I did a small rewrite to simplify the code.

91fa86eTrac #26141: new parameter align

I've introduced a new parameter to set alignment. (I hereby think of other separators than newline.)

e447b5eTrac #26141: trying to improve readability even more

I've factored out the padding in a separate function to obtain easier code-reading, I hope.

Please cross-review these changes.

comment:11 Changed 3 years ago by git

  • Commit changed from e83534113d56ef5d80f3c02a517c8e3bee572e96 to 31765a5b15e3f7611c55d184857376a9b2350491

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

31765a5Trac #26141: add docttest for align-parameter

comment:12 Changed 3 years ago by dkrenn

So, I am done; let's also see what the patchbot says ;)

comment:13 Changed 3 years ago by jipilab

  • Authors changed from Jean-Philippe Labbé to Jean-Philippe Labbé, Daniel Krenn
  • Reviewers changed from Daniel Krenn to Daniel Krenn, Jean-Philippe Labbé
  • Status changed from needs_review to positive_review

The bot seems happy, and I am too.

Thanks for further improvements. I set it to positive review as the issues seem to be fixed.

Best, JP

comment:14 Changed 3 years ago by dkrenn

Thank you very much for your work here on this ticket and on the original #24837; I appreciate it very much and like the improvements.

comment:15 Changed 3 years ago by vbraun

  • Branch changed from u/dkrenn/26141 to 31765a5b15e3f7611c55d184857376a9b2350491
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.