nansencenter / DAPPER

Data Assimilation with Python: a Package for Experimental Research
https://nansencenter.github.io/DAPPER
MIT License
348 stars 122 forks source link

UncertainQtty unexpected result #40

Closed yumengch closed 3 years ago

yumengch commented 3 years ago

Bug Description:

Motivated by the output in test_example_2.py, using 7.7676 ±1.2464 as input:

>>> from dapper.tools.rounding import UncertainQtty
>>> uq = UncertainQtty(7.7676, 1.2464)
>>> print(uq)
UncertainQtty(val=8, prec=1.0)

This is in contrast to the expected UncertainQtty(val=8.0, prec=1.0) or UncertainQtty(val=8, prec=1). I think the latter is preferred.

Cause of the bug:

Class UncertainQtty prints the first significant figure of the prec with the following code:

>>> np.round(1.2464 ,0)
1.0
>>> np.round(0.2464 ,0)
0.0

Possible Solutions:

Turn float to int if the first significant figure is ahead of the decimal separator.

Note

This issue is related to an old version TODO, which speculates the use of uq in the xps.tabulate_avrgs and xpSpace.print.

The difficulty is that, uq cannot be used directly for the output because it does not allow the designated decimals for its output. test_example_2.py has examples for decimals = 4.

Any comments?

patnr commented 3 years ago

I agree, your second expected output is the right one.

Possible Solutions:

Turn float to int if the first significant figure is ahead of the decimal separator.

I would say "...if the precision's decimal order is non-negative, i.e. if n>=0 in the __str__ method."

I believe the mentioned TODO was fixed by the previous update to rounding/UncertainQtty, except for this issue here which you found (very good attention to detail BTW). Is there something else I'm missing?

yumengch commented 3 years ago

I would say "...if the precision's decimal order is non-negative, i.e. if n>=0 in the str method."

Good point. I will add it.

I believe the mentioned TODO was fixed by the previous update to rounding/UncertainQtty, except for this issue here which you found (very good attention to detail BTW). Is there something else I'm missing?

I thought you planned to have 0.030 ±0.005 instead of 0.03±0.005. This requires padding 0 to the end of the string.

patnr commented 3 years ago

I thought you planned to have 0.030 ±0.005 instead of 0.03±0.005. This requires padding 0 to the end of the string.

Yes, you're right. I had forgotten that printing in the table does not yield the same nice result as printing individual UncertainQtty. Seeing as this was already complicated, I think it might get even more complicated. I suppose instead of
this one would have to call uq.__str__ and thereafter manually split(whitespace) and align on the point...

yumengch commented 3 years ago

I suppose instead of this one would have to call uq.__str__ and thereafter manually split(whitespace) and align on the point...

Yes. I am trying to add some code in the unpack_uqs and align_col to pad 0s as I feel the problem is not entirely in the UncertainQtty and I am just not sure if it is a good idea to give the option of deciding output decimals in UncertainQtty, which is decided automatically by prec at the moment. I just hope things won't get too ugly afterwards...

patnr commented 3 years ago

I feel the problem is not entirely in the UncertainQtty

I would say the problem is now not at all in UncertainQtty, which works fine (after casting to int).

I think the mult kwarg can be eliminated from UncertainQtty. It does not make much sense to keep it there. Any formatting not entirely determined by prec should be done elsewhere. Similarly UncertainQtty should not print to a requested number of decimals. That should be the job of the table formatter.

So if you want to revise the table formatting, remember these aspects:

All in all, not a very easy problem I think