adding functions to manage rounding

[cippy]

All changes in hepdata_lib/helpers.py
These functions are mostly utilities to facilitate rounding of values stored in dictionaries as returned, e.g., by RootFileReader::read_hist_1d()
The main one is roundValueAndUncertainty(), which allows to round values according to the significant digits of their respective uncertainties (rounded to a given input precision).

[clelange]

Hi @cippy, thanks a lot for this contribution! The tests show some failures, one related to the rounding, the other one simply being a linter error. Do you think you will be able to fix them yourself or would you like some help with it?

Do you think you could come up with some tests for the new functions? Also here: if you would like some help or advice, let me know!

[cippy]

Hi,
I think the error concerning the message comes from having modified line 64 to have non-negative values as second argument of the round function, i.e.
absolute_digits = max(0,-value_precision + relative_digits)
I think I got the new line looking at another branch and assumed it was the best case
Now, I see the test is actually allowing for absolute_digits to be negative, which thus breaks the test.

I see other errors are related to style in naming variables or not using spaces after commas, will try to fix them and push again.

For the new functions, I think I should be able to devise a test in the 'tests/' folder

Marco

[cippy]

I also noticed an error message during the tests, stating "itertools has no 'izip' member".
This arises from the fact that in Python 3 itertools does no longer have the izip member, and the built-in zip should be used, which does the same job as itertools.izip was doing in older verisions.
However, I was using the code from lxplus where python 2.7 is available.

A possible workaround I found online could be to just use zip, but import itertools as in the following
"
try:
from itertools import izip as zip # work in python < 3
except ImportError:
pass # python is >= 3.x, no need to import itertools
"

[clelange]

I also noticed an error message during the tests, stating "itertools has no 'izip' member".
This arises from the fact that in Python 3 itertools does no longer have the izip member, and the built-in zip should be used, which does the same job as itertools.izip was doing in older verisions.
However, I was using the code from lxplus where python 2.7 is available.

A possible workaround I found online could be to just use zip, but import itertools as in the following
"
try:
from itertools import izip as zip # work in python < 3
except ImportError:
pass # python is >= 3.x, no need to import itertools
"

I think this approach is reasonable. We do something similar at https://github.com/HEPData/hepdata_lib/blob/master/hepdata_lib/__init__.py#L17-L21.

However, we now have six as a requirement for the tests (see https://github.com/HEPData/hepdata_lib/blob/master/setup.py#L40-L41), and I think there's no harm actually making it a full dependency. You could then work around this issue by simply doing

from six.moves import zip

Your suggestion is equally valid though and I have no strong opinion.

[clelange]

@cippy Thanks a lot for your efforts! I made a few changes to test_helpers.py so that pylint doesn't complain anymore. The changes are made directly to your branch, so you can just fetch from your remote and merge (or simply pull).

[clelange]

I added a few comments and suggestions. Thank you for providing comments to document the new functions you implemented!

It would be nice if you added some smaller tests of the individual new functions you added. At the moment, you test round_value_and_uncertainty, which make use of the other new helper functions, but not the helper functions themselves. If you don't have time for that, let me know, and we can merge this now and create an issue to add more tests at a later point.

[clelange]

Should these lines actually use [index] to check each individual item? I changed that with my last commit in lines 49-50, but wanted to ask what was your intention here.

[cippy]

Your are right, initially I was thinking of making a check item by item, but eventually I forgot to add "[index]" after each set of square brackets. However, I think the direct comparison of the arrays is just equivalent and even simpler, the loop could be removed

[cippy]

Hi, sorry for the spam and the many commits to fix pylint errors.
Now it seems everything works fine except for tests/macospy3, but I have no idea why it happens. Perhaps you can better understand the origin of it.

With the last commits I implemented all the comments received above.
For the function get_value_precision_wrt_reference(), I eventually decide to accept only floats and ints as input arguments. I was initially implementing the case with both tuples or tuple (first argument) and float/int (for second one), but it required many more checks inside the function, and in any case the difference operator is not defined for tuples, so the elements should be subtracted one by one.
Thus, since for now the function was being used only to deal with floats, I just decided to go for the easier way. A user can deal with tuples by just using this simple function for each element, which seems easier than allowing for more complex behaviour inside the function itself.

I also added a fix to relative_round(), where I moved the check for the tuple input type as the first one. The problem was that if type(value) == tuple and the check is first made for 'value == 0', a ValueError is thrown, because 'The truth value of an array with more than one element is ambiguous'.
The same happens in get_number_precision() (there is also a comment to warn about this potential issue)

[clelange]

Thanks for your work! I checked and can confirm that the failing test for MacOS is not due to your code, but rather other things going wrong with the pyROOT installation there.
From my side the code looks good to me. We should probably think about how we could document this in a more prominent way.
What do you think @AndreasAlbert ?

[clelange]

@AndreasAlbert I'd merge this early next week unless you have additional comments. Let me know should you need more time.

[AndreasAlbert]

@clelange @cippy thanks for the work! I don't have further comments. Apologies for holding this up, I missed the mention.