Skip to content

Conversation

@axil
Copy link
Contributor

@axil axil commented Jun 8, 2022

Fixes #21695.

Copy link
Member

@HaoZeke HaoZeke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm faintly -0 on this. It seems like a verbose special casing (in code) for what is unambiguous to begin with.

@axil
Copy link
Contributor Author

axil commented Jun 8, 2022

@HaoZeke Not sure what you mean. Here's the existing code special casing the very same thing in latex representation:

if i == 0:
return '1'
elif i == 1:
return arg_str
else:
return f"{arg_str}^{{{i}}}"

I just did the same for string representation.

Can you elaborate?

Copy link
Member

@HaoZeke HaoZeke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following precedent as noted by @axil, this looks alright. Pending fixes to the tests of course.

@charris charris changed the title fix first power in str polynomials representation MAINT: fix first power in polynomials display. Jun 8, 2022
@charris
Copy link
Member

charris commented Jun 8, 2022

Lot of failing tests that expect the old style.

@axil
Copy link
Contributor Author

axil commented Jun 8, 2022

Lot of failing tests that expect the old style.

@charris Yes, there's some work to do. I wanted to get principal approval at the meeting to understand that I won't do meaningless work fixing the tests. I'll fix them today.

@axil
Copy link
Contributor Author

axil commented Jun 8, 2022

The tests proved to be deeply interconnected with the PR for #21653, so even if I make the tests pass in this PR it will result in a nontrivial merge conflict with that PR. So either
#21653 should be merged first, then this PR should be rebased and tests fixed accordingly or
– these two issues should be united into one.

@mattip
Copy link
Member

mattip commented Jun 9, 2022

PR #21654 to fix issue #12653 is failing tests. If it would be easier for you, I think it would be fine to merge the two changes into one PR. If it is easier to first concentrate on #21654 that is fine too.

@axil
Copy link
Contributor Author

axil commented Jun 9, 2022

Yes, this PR may be closed now. I'll merge fixes for both issues into the PR #21654.

@mattip mattip closed this Jun 9, 2022
axil pushed a commit to axil/numpy that referenced this pull request Jun 9, 2022
charris pushed a commit that referenced this pull request Jun 14, 2022
)

* limit the number of decimals in Polynomial representation

* tests pass

* parenthesize exponential notation in polynomials

* fixed a long line warning

* added polynomial printoptions tests

* polynomial printoptions typo fixed

* made switch to exp notation in polynomial display more natural

* added a test on switching polynomials to exp notation

* fixed linter errors/warnings

* support for nanstr and infstr printoptions in polynomials

* 10^8 threshold for switching to exp notation when displaying polynomials

* merged in PR #21696 fixing issue #21695

* made linter happy

* made some docstring tests pass

* fixed the docs

Co-authored-by: Lev Maximov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ENH: Replace x**1 with x in polynomials str representation

4 participants