belchertown.py: Losing precision by converting chart options to integer

[ShadowsFriend]

weewx-belchertown/bin/user/belchertown.py

Line 1148 in 507bbf2

output[chart_group][plotname]["series"][line_name] = self._highchartsSeriesOptionsToInt(output[chart_group][plotname]["series"][line_name])

This line converts the value of options not covered in the code beforehand to integer. Unfortunately, this leads to loss of precision for options that can actually reasonably be floats like yaxis_tickinterval. Looking at the generated JSON for the pressure chart from graphs.conf.example[1] shows the value for tickInterval, which is supposed to be 0.01, getting truncated to 0, for example.

As I was not sure how you would like to approach the issue, I decided to open this report instead of a pull request right away. My suggestion would be converting everything to float altogether. As JavaScript has only one number type, which is a 64-bit floating point type, anyway, Highcharts, as a JavaScript library, should be perfectly fine with floats passed for any option requiring a number.

[1] The yaxis_tickinterval specified for the pressure chart in the graphs.conf.example is capitalized in a different way than belchertown.js expects it. belchertown.js checks for yaxis_tickinterval but the file contains yAxis_tickInterval which does not work no matter the value. This is, however, unrelated to the issue and even with the correct capitalization 0.01 gets truncated to 0.

Edit: A bit of rewording.

[poblabs]

Thanks again for taking a look!

I added the self._highchartsSeriesOptionsToInt() function because ConfigObj turns everything into a string, and I know JavaScript wanted certain configs (like the yaxis) as an integer and not string. Perhaps I should have gone float as you've explained.

If you want to submit another PR which cleans that up, that'll be appreciated. I think _highchartsSeriesOptionsToInt should also get renamed to _highchartsSeriesOptionsToFloat?

If in the same PR you want to fix my typo on the yAxis_tickInterval that's helpful too. To be honest, all of these options should probably be converted to lowercase so they are consistent no matter what the user enters.

[poblabs]

If you are going to submit a PR, you should import weewx's to_float and use that to generate the float (much like to_int is doing now).

[ShadowsFriend]

If in the same PR you want to fix my typo on the yAxis_tickInterval that's helpful too. To be honest, all of these options should probably be converted to lowercase so they are consistent no matter what the user enters.

Reworking the option naming is something I have thought about as well. It would be really cool to be able to use as many Highcharts options as possible, for example alignTicks, without the need for them to be explicitly handled somewhere in the skin's code. The problem is, that we somehow need to get the correct data types from the strings and converting everything to lowercase would further complicate the issue by making it necessary to maintain a mapping between our option names and the camelcase names used by Highcharts even for options that would normally not need any special processing and could just be passed along.

An idea for solving the first issue might be to introduce some kind of type annotation in graphs.conf for options that have no explicit handling in the code. Something like alignTicks = false:bool which would then allow the JSON generator to simply check for the annotation to know what the strings should be converted to. There might be other options I do not know about yet, though. For the second issue it might be worth considering using the Highcharts naming for all the options throughout, instead of switching to lowercase for everything. This would allow to simply pass options that need no explicit processing.

Edit: The second issue would also be solvable by only converting options the code actually knows about to lowercase before further processing it. The unknown options can then simply be passed on.

[poblabs]

When I started out to make the charts more user-customizable, I took a note from weewx's ImageGenerator. I liked how Tom had the option leaves setup and everything works pretty well.

I wanted to try and keep a mapping out of it. And that was possible with the Series options since those are just nested dicts, but for other items like the yaxis min/max/softmin/softmax, gapsize, connectnulls, etc. I had to maintain the mapping. So I told myself that if I was going to maintain mapping then I should follow the camelCase that Highcharts is using. So I failed there with the yAxis_tickInterval being lower since Highcharts uses yAxis.

But then I had the idea that the average user probably doesn't care about camelCase, they just want their stuff to work. Somewhere in that thought process I guess I veered off a bit and fragmented things. So I agree, something like yAxis_tickinterval should be yAxis_tickInterval.

(If you're bored, 😃 You can read a lot about my thoughts during the development process in some of the 1.0 milestone issues where we were testing.)

I also don't want to maintain a mapping of all of Highcharts options in my code since that seems redundant and a lot of overhead (for example, when they add a new feature, Belchertown has to add the ability to know that feature exists)

I agree some sort of way to remove the mapping would be great but as it sits today I'm not sure that's possible. So I've been adding mapping to the code if/when someone asks for it.