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

Future of units.constants #2230

Open
PicoCentauri opened this issue Apr 1, 2019 · 7 comments

Comments

@PicoCentauri
Copy link
Contributor

commented Apr 1, 2019

As we discussed in #2118 it would be less code to maintain if we replace the units in the constants dictionary by imported values from SciPy.constants. This is trivial to change but maybe this dictionary is superfluous at all? I found no other place in the code where the dictionary is used and lines inside the units.py could also directly take the values from SciPy.

I also think it is probably out of the scope of MDAnalysis to provide a list of physical constants to the user.

Summary: I would suggest to replace the values in the constants dict by the values from SciPy and simultaneously deprecate the whole constants dictionary. What do you think?

@orbeckst

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

I echo the conclusions of #2118 (review) – now that scipy is a dependency, we should use it.

  • check that scipy and MDAnalysis.units contain the same values
  • add tests that test that N_Avogadro, elementary_charge, calorie still have the same values that we had, just so that we find out when something changes downstream. We don't want to silently get different results just because scipy updated values...
  • remove the constants dict
  • remove N_Avogadro

Perhaps we can get rid of units completely – if someone can find out what good unit packages are. We would then have to change various other parts of the code. That would be a small project in itself.

@PicoCentauri

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

The first thing I already did and only the Avogadro number differs by -4.32e16 which is a relative error of ~10^-7 and should be fine.

I'm confused by your last two points. Do you suggest to remove the dict and N_A directly or first deprecate them?

@orbeckst

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@mattwthompson

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Perhaps we can get rid of units completely

Just chiming in with some personal experience: lately I have liked unyt and used it a lot, but pint is also very popular. There are many other unit modules scattered across other packages but I like the idea of using a package that handles units and handles units only (less likely for me to screw things up and less code for me to re-write, and it gets a little more attention than buried as a module in a larger package). In my case I was part of building something from the ground up so I did not have to consider the cost of refactoring.

https://github.com/yt-project/unyt (see their paper for some benchmarks: http://joss.theoj.org/papers/10.21105/joss.00809)
https://github.com/hgrecco/pint

@orbeckst

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@orbeckst

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

unyt looks good although pulling in sympy as a dependency seems a bit much. But maybe other people have other opinions.

I am not sure if we really need to do all our computations on arrays with units, which has performance penalties, according to the unyts paper. So far we have been quite good at controlling units at the I and O points and we know what our internal unit system is. If we adopt a proper unit package then perhaps we just use it to calculate the conversion factors? In any case, benchmarking will be necessary.

@richardjgowers

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.