e-mission / em-public-dashboard

A simple and stupid public dashboard prototype.
BSD 3-Clause "New" or "Revised" License
1 stars 23 forks source link

Distinguish Imperial vs Metric Units based on the config #114

Closed Abby-Wheelis closed 7 months ago

Abby-Wheelis commented 7 months ago

Adapting chart generation and display to use the units requested in the config by the display_config.use_imperial field

This process will affect the chart generation notebooks and the html files, since some of the units are hard coded there, and we need to be fetching another parameter from the config file to be fed to the display and chart generation

This PR also serves as my first effort working on this side of the dashboard.

Addresses #108

Abby-Wheelis commented 7 months ago

I have now addressed unifying the if statements, removing extra cells, and reverting extraneous commits - I tried reverting the commit where most of that came in, but it broke several of the notebooks, so I removed them manually (I know this left the commit history a a mess but hopefully cleaned up the diff)

I still need to test everything again with generate_plots.py, but have run out of time today

Abby-Wheelis commented 7 months ago

I ran through the testing and found several bugs which popped up when there was no data, no replaced modes, etc

I have been able to resolve these bugs so the notebooks run without error and the built in error handling can represent the errors smoothly.

Testing was done by executing PYTHONPATH=.. STUDY_CONFIG=cortezebikes python bin/generate_plots.py energy_calculations.ipynb default for each of the notebooks in the crontab. After fixing the bugs I found, each runs smoothly.

I then check the dashboard interface from it running locally, and found one more error, which I fixed in 36fdb88, and now all the charts are showing up as expected, except for Average trip length and Average trip length (sensed) which is known and under construction in #113.

shankari commented 7 months ago

@Abby-Wheelis

so I removed them manually (I know this left the commit history a a mess but hopefully cleaned up the diff)

This is fine, I can just squash merge to unify the commit history

Testing was done by executing PYTHONPATH=.. STUDY_CONFIG=cortezebikes python bin/generate_plots.py energy_calculations.ipynb default for each of the notebooks in the crontab. After fixing the bugs I found, each runs smoothly.

Just to clarify wrt https://github.com/e-mission/em-public-dashboard/pull/114#discussion_r1483239483 if you didn't revert the changes, the notebook would still run, but it would not override the month and year values. So we would end up with the same graphs for "All Data" and for each month. Can you confirm that this is not happening?

Can you confirm that this is not happening in your most recent set of tests?

Finally, I will review this now to give you feedback, but you need to resolve the merge conflicts from the previous merge of #113

Abby-Wheelis commented 7 months ago

Can you confirm that this is not happening in your most recent set of tests?

After resolving merge conflicts, I tested 1 more time and confirmed that charts are different month-to-month [I fixed the way I had resolved the merge conflicts after I took these screenshots, the chart labels in the html are correct now]

Screenshot 2024-02-10 at 2 01 04 PM Screenshot 2024-02-10 at 1 59 51 PM
Abby-Wheelis commented 7 months ago

Centralized to scaffolding.py and addressed the other few comments.

When it comes to index.html, I think that a reformat PR is the best route. Throughout the file the indentation style is inconsistent - going back and forth between 2 space and 4 space tabs in some places, extra indents that seem random (at least to me), I've submitted a single-commit PR #119 to take care of reformatting the whole file to have consistent formatting. I think this is a better change then trying to match the old way (which I did try to do and kept making it worse).

shankari commented 7 months ago

@Abby-Wheelis everything looks fine except for index.html which still seems really messy?!

Abby-Wheelis commented 7 months ago

@shankari I saw that when the merge for the formatting changes went through earlier and thought toggling the base would help but it did not, I'll resolve when I get the chance today, it should be the case that only my additions show in the diff for that file.

Abby-Wheelis commented 7 months ago

Now the diff for index.html only shows changes that occurred as a part of this change - it was some combination of issues between GitHub and VScode