Skip to content

BUG: Polynomials now copy properly (#22669)#22670

Merged
mattip merged 2 commits intonumpy:mainfrom
MatteoRaso:master
Nov 27, 2022
Merged

BUG: Polynomials now copy properly (#22669)#22670
mattip merged 2 commits intonumpy:mainfrom
MatteoRaso:master

Conversation

@MatteoRaso
Copy link
Copy Markdown
Contributor

On line 502, self.symbol.copy() was called, which
causes an AttributeError, since self.symbol is a
string, so it doesn't have a copy() method. To fix it,
I simply removed the copy() and directly assigned the string.

On line 502, self.symbol.copy() was called, which
causes an AttributeError, since self.symbol is a
string, so it doesn't have a copy() method. To fix
it, I simply removed the copy() and directly assigned
the string.
@mattip
Copy link
Copy Markdown
Member

mattip commented Nov 25, 2022

I wonder if at one point symbol was an ndarray, but then got changed into a string. Is there a pickle test for Polynomial?

@MatteoRaso
Copy link
Copy Markdown
Contributor Author

MatteoRaso commented Nov 25, 2022

I wonder if at one point symbol was an ndarray, but then got changed into a string. Is there a pickle test for Polynomial?

No. I checked the commit history, and the symbol attribute was added in commit b84a53d as a string, which is also when the bug was introduced.

@mattip
Copy link
Copy Markdown
Member

mattip commented Nov 26, 2022

Could you add another test that checks y = pickle.loads(pickle.dumps(x)); assert_equal(x, y) (it fails on main)?

Otherwise LGTM. xref @rossbar

@MatteoRaso
Copy link
Copy Markdown
Contributor Author

Could you add another test that checks y = pickle.loads(pickle.dumps(x)); assert_equal(x, y) (it fails on main)?

Otherwise LGTM. xref @rossbar
Done.

@mattip mattip merged commit 343fae6 into numpy:main Nov 27, 2022
@mattip
Copy link
Copy Markdown
Member

mattip commented Nov 27, 2022

Thanks @MatteoRaso

@MatteoRaso
Copy link
Copy Markdown
Contributor Author

No problem.

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.

4 participants