brightway-lca / brightway2-parameters

Library for storing, validating, and calculating with parameters
BSD 3-Clause "New" or "Revised" License
1 stars 2 forks source link

Pint cleanup #6

Closed cmutel closed 1 year ago

cmutel commented 1 year ago

@BenPortner Thanks for the work in incorporating Pint. I have merged the PR, but after playing with this a bit more, I think we need some substantial cleanup before making a release. I tried to do this myself, but I am not able to understand some of the hidden assumptions (and there isn't any documentation yet), so am really struggling. I would like the following:

@tngTUDOR FYI

BenPortner commented 1 year ago

Hi @cmutel ,

thanks for merging my PR. The things you request are doable but some of them I wouldn't recommend. Here are my thoughts:

Make pint required

I agree.

Remove compatibility layers and config

Don't test if pint is available: fine. But I don't think it's a good idea to remove the config. I would like to give the user the possibility to switch off the pint parser when using bw2data.parameters. This is because some of the default units implemented in pint may overlap with the user parameter name space (e.g. a = year, b = barn, c = speed of light). This should not be a problem if the parameters are properly defined but may lead to unexpected results if parameters are renamed or removed and not all dependent formulas are updated.

Rename DefaultInterpreter and DefaultParameterSet

Not sure Interpreter is a good choice because then bw2parameters.Interpreter will overlap with asteval.Interpreter. Are you suggesting this because you want PintInterpreter/PintParameterSet to be the default or is there another reason?

Change tests from classes to pure functions

The reason why I introduced classes is because this way I can re-use the tests of the (default) Interpreter for the PintInterpreter. I want to make sure that the PintInterpreter behaves exactly the same as the (default) Interpreter for the base cases. I don't know how to achieve this with pure functions.

Remove Py2 compatibility stuff in utils

Agreed.

Removal of PintWrapper

Removal is not recommended. We need one central UnitRegistry instance, otherwise unit parsing becomes very slow. Should I rename it to pint_wrapper.py?

Documentation

cmutel commented 1 year ago

Thanks for your detailed response, and for all the work you have done here.

I agree with each of your conclusions. You can decide on the naming of PintWrapper, probably easiest to leave it alone as it works.

cmutel commented 1 year ago

Fixed in https://github.com/brightway-lca/brightway2-parameters/commit/00dc1925889023813d6df5c35d070aa7ce396ca3