Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace quantities with pint #9

Merged
merged 4 commits into from
Aug 12, 2013
Merged

Replace quantities with pint #9

merged 4 commits into from
Aug 12, 2013

Conversation

matze
Copy link
Contributor

@matze matze commented Aug 9, 2013

The quantities package is nice "but extremely slow". This

timeit.timeit("b=a.simplified.magnitude", setup="import quantities as 
q;a = 123 * q.mm", number=1000)/1000.0

results in 0.5 ms per conversion. Moreover, quantities has a hard dependency on Numpy which I don't like too much.

This change replaces quantities with pint:

  • To hide future changes, I added concert.quantities which exports one module variable called q. In the future we can easily exchange the implementation without requiring users to change their code. But this also means, that we have to change the current imports from import quantities as q to from concert.quantities import q.
  • pint is a bit more picky about "type safety", e.g. comparing quantities against simple numbers is not possible anymore.
  • pint also does not support number per unit quantities, so we need to replace something like 1 / q.mm by either q.dimensionless / q.mm or q.count / q.mm. For motors I prefer the latter, because counts should easily map to steps.
  • pint is only working for Python 2.7+ but fully supports Python 3.

@matze
Copy link
Contributor Author

matze commented Aug 9, 2013

@sensej: could you check why it hangs a bit during the alignment test? With pint it now also happens with Python 2.7 and with Travis CI.

@matze
Copy link
Contributor Author

matze commented Aug 9, 2013

@sensej I fixed the reason why it got stuck. Although all tests pass, I am not too sure about my changes. Could you review them?

@tfarago
Copy link
Contributor

tfarago commented Aug 10, 2013

The concert.quantities is a good idea, I just hope we will not end up implementing our own quantities in the future.

@tfarago
Copy link
Contributor

tfarago commented Aug 10, 2013

Maybe we should give it a second thought and when we decide to go for it we have to fix all the tests, because round(a - b), when a and b are pint quantities does not work anymore and one finds this problem e.g. in unittest.TestCase.assertAlmostEqual, see also https://travis-ci.org/ufo-kit/concert/builds/10055032.

@matze
Copy link
Contributor Author

matze commented Aug 10, 2013

It works, but the quantities must be declared that they contain a Numpy array

np.round(np.array([2.3]) * q.mm)

will work. One the one hand I think this is a good idea, because it forces people (and us) to think about what they are doing. One the other hand, it's definitely more work on some parts.

@matze
Copy link
Contributor Author

matze commented Aug 12, 2013

@sensej There are still four tests failing and I think they are because we use the .units thing in the optimization algorithms. However, I am too stupid to see how to fix this.

@tfarago
Copy link
Contributor

tfarago commented Aug 12, 2013

It came from the fact that pint quantities are more strict, e.g. you cannot do 0 == 0 meter.

@tfarago
Copy link
Contributor

tfarago commented Aug 12, 2013

Ha! You can, but you cannot do 0 < 0 meter. It's correct behaviour but one really needs to think hard about what they are doing, which is probably good like you already said anyway.

@tfarago
Copy link
Contributor

tfarago commented Aug 12, 2013

There should also be a UnitRegisty.wraps method (http://pint.readthedocs.org/en/latest/wrapping.html), but that doesn't work for me and I have the current release. It does something similar as https://github.com/ufo-kit/concert/blob/mv/pint/concert/optimization/algorithms.py#L62-74.

@matze
Copy link
Contributor Author

matze commented Aug 12, 2013

Yeah that wrapping looks really what we need.

If it's really not your fault, you could file a bug about your problems. pint is also on GitHub.

@matze
Copy link
Contributor Author

matze commented Aug 12, 2013

But back to a more strategic topic: How should we proceed? Does pint fix your performance issues?

If you are okay with merging into master, I will squash some commits and review the docs.

@tfarago
Copy link
Contributor

tfarago commented Aug 12, 2013

In fact it does not matter (performance-wise). If you convert to the base units first, pint and quantities are similar in performance. But I also dislike the numpy dependency. So, if there are no other crucial hiccups let's use pint.

@tfarago
Copy link
Contributor

tfarago commented Aug 12, 2013

Actually, when you convert a numpy array and its size is less than million elements pint is faster than quantities, but as the array grows larger it gets the other way around.

@matze
Copy link
Contributor Author

matze commented Aug 12, 2013

Actually, when you convert a numpy array and its size is less than million elements pint is faster than quantities, but as the array grows larger it gets the other way around.

My gut tells me that we handle way more mini arrays with units than big ones.

matze added a commit that referenced this pull request Aug 12, 2013
Replace quantities with pint
@matze matze merged commit c1ba44d into master Aug 12, 2013
@matze matze deleted the mv/pint branch August 12, 2013 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants