fix bad code quality for recently merged #24837

[dkrenn]

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://github.com/sagemath/sagetrac-mirror/commit/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 = -...

   ?

8. 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.

9. 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?

10. 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?

11. 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.
12. 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.)

Depends on #24837

CC: @jplab @tscrim @mo271 @vbraun @videlec

Component: geometry

Author: Jean-Philippe Labbé, Daniel Krenn

Branch/Commit: 31765a5

Reviewer: Daniel Krenn, Jean-Philippe Labbé

Issue created by migration from https://trac.sagemath.org/ticket/26141

[jdemeyer]

comment:1

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.

2. A fix for the code.

3. A revert of the code.

[tscrim]

Description changed:

--- 
+++ 
@@ -59,9 +59,3 @@
    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?
 21. 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.
 32. 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.)
-
-#24837 has gone in 8.4.beta0 and no sign so far for 8.4, so there is still time to fix this. As it is code that should not have been beta-released and there is clearly something to do before the next release, I consider this a blocker ;)
-
-Therefore, dear authors and reviewers of #24837, please fix this until the next stable release. (I am happy to do the reviewing.)
-
-PS: If someone finds that I am overacting feel free to bring me down to earth; it won't be too hard, i promise ;)

[tscrim]

comment:2

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.

[dkrenn]

comment:3

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.

[jplab]

comment:4

Hello,

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

I am on it.

[dkrenn]

comment:5

Replying to @jplab:

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 :)

[jplab]

Commit: d7ec556

[jplab]

comment:6

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.

3. 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.

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(...))

Thanks for mentioning, forgot to look over that.

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(...).

Done.

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. ;) )

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.

7. Why

   product = -1*(coeffs[1:]*vars[1:])

   and not simply

   product = -(...)

   or even

   product = -...

   ?

Done.

8. 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.

9. 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.

10. 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.

11. 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 '>='.

12. 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:

d7ec556

Fix bad code quality of 24837

[jplab]

Branch: u/jipilab/26141

[jplab]

Author: Jean-Philippe Labbé

[dkrenn]

Changed branch from u/jipilab/26141 to u/dkrenn/26141

[dkrenn]

Changed commit from d7ec556 to e835341

[dkrenn]

comment:9

Replying to @jplab:

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.

3. 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.

4. 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.

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(...).

Done.

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

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. ;) )

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.

7. Why

   product = -1*(coeffs[1:]*vars[1:])

   and not simply

   product = -(...)

   or even

   product = -...

   ?

Done.

LGTM.

8. 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.

9. 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.

10. 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.

11. 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.)

12. 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:

b83b52b	Trac #26141: one PEP8 space after comma
9ecefa0	Trac #26141: code simplification split/length
91fa86e	Trac #26141: new parameter align
e447b5e	Trac #26141: trying to improve readability even more
7967bcf	Trac #26141: a PEP8 line break and space-comma
e52b936	Trac #26141: align docstring and remove comma
e835341	Trac #26141: rewrite OUTPUT block to avoid doubling information on style-parameter

[dkrenn]

comment:10

Replying to @dkrenn:
9ecefa0	Trac #26141: code simplification split/length

I did a small rewrite to simplify the code.

91fa86e	Trac #26141: new parameter align

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

e447b5e	Trac #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.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from e835341 to 31765a5

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

31765a5

Trac #26141: add docttest for align-parameter

[dkrenn]

comment:12

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

[jplab]

Changed reviewer from Daniel Krenn to Daniel Krenn, Jean-Philippe Labbé

[jplab]

comment:13

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

[jplab]

Changed author from Jean-Philippe Labbé to Jean-Philippe Labbé, Daniel Krenn

[dkrenn]

comment:14

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.

[vbraun]

Changed branch from u/dkrenn/26141 to 31765a5