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

Inconsistent use of locale #230

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

gu1lhermelp
Copy link
Contributor

This PR addresses the issue #229.

The changes were made in the set_display module of the PyDMLineEdit and PyDMLabel widgets.

The change consists of simply replacing the standard decimal separator '.' by the one defined in the locale settings.

Perhaps a more elegant solution would be to make this changes in the the PyDMWidget's update_format_string method, since that would affect all widgets with a similar behavior. However, at this time I could not think of an easy way to implement this changes, as that would entail having to use the locale module formatter instead of the .format currently being used.

Anyhow it seems to be working.

cheers,
Guilherme

…or `.` to the locale one when the value being shown is a float
…tor `.` by the locale one when the value being shown is a float
@gu1lhermelp gu1lhermelp changed the title Pr bugfix numeric locale Inconsistent use of locale Jan 30, 2018
@hhslepicka
Copy link
Contributor

Hi @gu1lhermelp ,
The PR looks great! Pinging @mattgibbs also into this thread.

Here is some info that we can discuss a little bit:

Python string format has the n presentation type which is:

Number. This is the same as 'g', except that it uses the current locale setting to insert the appropriate number separator characters.

Based on that and some tests I can see that it works but there is a small detail... We can't easily specify the number of decimal places (precision) without a small hack:

import locale

# an example value
val = 1235.21234

# let's switch the locale to pt_BR
locale.setlocale(locale.LC_ALL, "pt_BR")

print("{:.3n}".format(val)) # Will give us '1,24e+03'

# But if we do: 
print("{:.7n}".format(val)) # This will give us '1.235,212', which is the desired output.

# So for a general term we would need to always be generating the mask based on the "size" of the value. Something like:
fmt = "{:."+str(len(str(int(val))) + prec) + "}"
print(fmt.format(val)) # This will give us '1.235,212',

One other approach is to change the way the data is formatted at PyDM and provide a better and more uniform way to do so.

@xresende
Copy link
Contributor

@hhslepicka , your simple-approach solution example worked within ipython in my system. I set locale to pt_BR.utf8 and used

fmt = "{:."+str(len(str(int(val))) + prec) + "n}"

I did not know about n presentation type! thanks.

@gu1lhermelp
Copy link
Contributor Author

@hhslepicka nice suggestion! I added a new commit using the 'n' specifier instead of the old solution.

@xresende
Copy link
Contributor

@fernandohds564 just pointed out that fmt, as defined above, will have problems when value is negative. we will update the PR to deal with these situations. another issue: if prec == 4 and value == 1.2, for example, should we expect the string to be right-padded with zeros? I think this is desirable but the use of the n presentation type, as far as I understand it, does not paddle the string.

@xresende
Copy link
Contributor

this PR has problems with PV values between (-1,1)... we need to think more about it.

@hhslepicka
Copy link
Contributor

I cannot reproduce this issue, would you mind to clarify what is going on with those values?
Send me some examples of input and wrong output.

@gu1lhermelp
Copy link
Contributor Author

Hello Hugo here is an example:

val = 0.000123456
print("{:.5n}".format(val)) # this will give 0.00012346 and not 0.0001

@hhslepicka
Copy link
Contributor

What is happening is that for values starting with 0 the formatting is counting from the first non-zero digit... I will think about it too and if I come up with some ideas I will send it here.

@xresende
Copy link
Contributor

@hhslepicka , we have an idea on how to generalize string representations of PV values properly taking into account locale. We will only have time to work on this probably next week.

@klauer
Copy link
Collaborator

klauer commented Feb 21, 2018

Perhaps something like:

from decimal import Decimal
'{:n}'.format(Decimal('{:.5f}'.format(0.000123456)))
In [44]: '{:n}'.format(decimal.Decimal('{:.5f}'.format(0.000123456)))
Out[44]: '0.00012'

In [45]: import locale

In [46]: locale.setlocale(locale.LC_ALL, 'pt_br')
Out[46]: 'pt_br'

In [47]: '{:n}'.format(decimal.Decimal('{:.5f}'.format(0.000123456)))
Out[47]: '0,00012'

@gu1lhermelp
Copy link
Contributor Author

Hello all, in the last commits I am sending a new solution so you guys can evaluate it.

Basically, I removed the class member format_string. It was replaced by the get_formatted_string method. In this method the locale.format_string uses the locale setting when a float is passed. This change affected the label, line edit, scale and slider widgets.

As the format_string was removed the update_format_string was rendered useless, so I thought. But this method had another function besides updating the format_string. It was being redefined by some widgets (slider and spinbox), in order to indicate that an update, in the PV channel (value, unit, precision) or PyDMWidget property (showUnits), happened. So this method was kept to serve this particular need. Although the name does not fit anymore.

I tested the six widgets that were altered and they seem to be working fine.

I understand this is a major change as it alters the PyDMWidget interface, I await your comments and suggestions.

@fernandohds564
Copy link
Collaborator

Hi guys!
This PR has been here for a while...
@hhslepicka and @mattgibbs, did you guys have time to look at @gu1lhermelp solution?
If you think this approach is not appropriate, because it changes the interface of PyDMWidget, with your feedback we could try a different one.

Thanks.

@hhslepicka
Copy link
Contributor

Hi @fernandohds564.. sorry about that!
I am working on a PR that will come up soon to add support for Structured data at PyDM, which will enable the usage of PVAccess (aka Epics V4) and other data sources that are not one-to-one.
I am waiting a little to merge this PR since we will change the PyDMWidget interface by this time so my idea is to apply this change at the same time as my PR.
We are also working here at SLAC to increase the number of automated tests which will make it easier for all of us to diagnose the code changes.
This PR is not forgotten and by any reason it is an inappropriate solution. We just need a bit more of time to get this one in right.

@hhslepicka hhslepicka self-assigned this May 7, 2018
@hhslepicka hhslepicka removed their assignment Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants