Error bar #975
Error bar #975
Conversation
Also removed a commented out line of code.
There was a problem hiding this comment.
Overall, great work from @Elias0127 and @JesseVarGar. This is a good implementation of the work that was done on the backend. I've got a few small code styling comments, and there are also two issues with the implementation.
As noted below, the min/max points array is kept across meters, which leads to problems when displaying bars on multiple meters.
Additionally, there should not be error bars for meter-resolution data. Currently, the code tries to display error bars on this data, which leads to errors in the console as well as weird plotly output.
|
@Elias0127 From the records I have, you have yet to sign the CLA for OED. OED needs a signed CLA to accept this work. If you think my records are in error then please let me know. |
|
Before this PR I pushed a commit to get the tests working. I want to note that the fix did not include testing the new error bar values returned. I have opened issue #984 so the tests will be enhanced in the future. For now, the PR will be manually tested. |
|
There is a slightly annoying behavior where adding error bars to some meters shifts the slider bars for the time range. What is interesting is the actual range displayed is not altered but the slider can now be zoomed out to earlier and later dates. If you graph test15MinSin over all time with and without error bars you can see the effect. I tried to debug this and found that simply commenting out the include of the error_y but leaving all the calculations removes the shift. This was consistent with finding that the graphic values seemed the same for the values with and without the error bars except for the error ranges. I have looked at the data in the debugger and it only seems to change the y-axis values so why the x-axis is effected is unclear. It isn't a big issue and the graph is correct. I wanted to document this and also see if someone else has any ideas. |
Per discussion and agreement with the developers of this PR, I added the min/max error bars to the chart link ability.
|
Per agreement with the developers of this PR, I just added min/max to the chart link. It should work correctly now and reproduce the desired graphic. Others should test that it is correct and code is okay. |
|
The line graphic no longer shows min/max values on raw data. This is done by setting the values to null in Redux and NaN from the DB. The graphic shows each meter so if one is raw it will not show min/max but if another in the same graphic is not raw then the min/max is shown.
|
Given the comments by @spearec, I have removed min/max for any meter that is raw data. Other meters still show min/max. Again, others should check this out to agree that this is a good fix. |
There was a problem hiding this comment.
Congratulation to Elias and Jesse on their first PR to OED. Thanks for through work. My testing did not find any issues. I made a few comments that should be addressed. I also note:
- I made a few commits to the PR (see comments to PR) that others should look at and verify to be correct.
- I edited the description to remove the [ ] around the issue number so GitHub properly links.
Thanks to @spearec for catching my mistake. I missed all the zero value at the bottom of the graphic. I believe their fix is correct. |
|
I have reviewed all the requested changes for both myself and @spearec. Everything looks good and this PR is ready to merge. I appreciate the efforts of @Elias0127 and @JesseVarGar and their understanding for the time it took us to do the final reviews. |
Description
We grabbed the min and max data from the database and added it to the line graph as error bars. Note that we did not write the SQL query (src/server/sql/reading/create_reading_views.sql) that was responsible for creating the min and max data in the database. The code was already written, and we only uncommented it.
This is a joint work
Co-authored - by JesseVarGar
Fixes #594
Type of change
(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])
Checklist
(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)
Limitations
Adding min and max to the database caused a mismatch with the tests and resulted in them failing.