Skip to content

Fixed non-list tolist() bug#186

Merged
apdavison merged 3 commits intopython-quantities:masterfrom
scidash:master
Oct 18, 2021
Merged

Fixed non-list tolist() bug#186
apdavison merged 3 commits intopython-quantities:masterfrom
scidash:master

Conversation

@rgerkin
Copy link
Copy Markdown
Contributor

@rgerkin rgerkin commented May 28, 2021

Fixes #185.

import sys
sys.path.insert(0, '/home/rgerkin/Dropbox/dev/quantities')
import numpy as np
import quantities as pq

x = np.array([1.0, 2.0])
y = np.array(3.0)

x.tolist()  # Works fine
y.tolist() # Works fine

(x * pq.V).tolist() # Works fine
(y * pq.V).tolist() # Throws exception (fixed by this PR).

@rgerkin rgerkin mentioned this pull request May 28, 2021
Closed
@twmr
Copy link
Copy Markdown
Contributor

twmr commented May 30, 2021

Thx for this fix. Please also add a unit-test for it somewhere here

python-quantities/quantities/tests/test_methods.py

Line 13 in 6ee561e

self.assertEqual(self.q.tolist(), [[1*pq.m, 2*pq.m], [3*pq.m, 4*pq.m]])

@rgerkin
Copy link
Copy Markdown
Contributor Author

rgerkin commented May 30, 2021

I've updated the unit test to check this. An expected failure also succeeds now, although I think this might be due to a change (for the good) in numpy behavior, so I updated that test as well.

@rgerkin
Copy link
Copy Markdown
Contributor Author

rgerkin commented Jun 21, 2021

@Thisch Is there anything else you need from me or can this be merged and a new release created? Travis passes for all non-archaic numpy versions.

twmr
twmr reviewed Jun 21, 2021
View reviewed changes
quantities/tests/test_umath.py
[-1., -1., -0., 1., 2., 2., 2.] * pq.m
)

@unittest.expectedFailure
Copy link
Copy Markdown
Contributor

@twmr twmr Jun 21, 2021

Choose a reason for hiding this comment

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

Why was this change and the change below needed? It doesn't seem to be related to the bug fix in the tolist mehtod.

Copy link
Copy Markdown
Contributor Author

@rgerkin rgerkin Jun 21, 2021

Choose a reason for hiding this comment

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

@Thisch The numpy .fix method is something that we would like to behave the same in numpy and in quantities (i.e. it should round towards zero). It used to not work with quantities, which is why the expected failure was there. But it works now, because of changes in numpy I think (though it could be the result of my changes)? So I removed the expected failure because it now passes as it should.

@twmr
Copy link
Copy Markdown
Contributor

twmr commented Jun 21, 2021

I've updated the unit test to check this. An expected failure also succeeds now, although I think this might be due to a change (for the good) in numpy behavior, so I updated that test as well.

I see.

Thx for the fix and the unittest. IMO this is ready for being merged.

@apdavison
Copy link
Copy Markdown
Contributor

apdavison commented Jun 22, 2021

I'd like to update the test matrix to remove older versions of NumPy, but we need to migrate to travis-ci.com first (see #187)

@apdavison apdavison merged commit 269d81e into python-quantities:master Oct 18, 2021
@rgerkin rgerkin mentioned this pull request Feb 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@twmr twmr twmr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tolist() fails on non-list quantity

3 participants

@rgerkin @twmr @apdavison