Closed MaanasArora closed 5 months ago
Awesome, thank you Maanas! I changed the base to development
branch instead (you will need to merge with the new updates there, it's ahead of main
). That is where we collect new updates - main
is reserved for deployment.
Also, if you could add a few bullet points about your changes how the implementation was done, it would make the review a ton easier. Also, please note that the ticket you reference was about showing these indicators for the device level data.
I wonder if there is a chance to actually get away with a single data loader for the system stats (one with the time-of-day markers) and use that across the system-wide indicators. Thanks again 🙏
Thanks for the review @danieltsoukup! Sorry for missing that the indicators were device-level. I can open a new PR for that too.
Here's some bullet points for how my implementation was done:
AbstractIndicatorPlotter
called TimedMinAverageIndicatorPlotter' that is identical to
MinAverageIndicatorPlotter` except:
time_of_day
, which is a string, and filters by time_of_day
when it is getting the average in _get_min_avg
.I tried to adhere to the design principles of the application as much as possible, but as you noticed, there is a lot of additional processing. I conceptualize the issue we are having as a blurry line between the responsibilities of the (lower) SQL processing and the (higher) Pandas processing. For example, we are querying the WebCOMAND server multiple times for data that comes from the same source of truth.
If internet usage is the concern, note how the minimum amount of data we have to get from the server is bounded by the size of the hourly data query, so I would recommend we establish a hourly query as the source of all our data and do Pandas operations on top, which removes the need for redundant processing. Then we can also make our architecture much simpler.
Would you be interested in pursuing this? I can open a PR if you would like. Thanks a lot!
Hi! Sorry for not letting you know I was doing this, I actually just started 2 hours ago.
This change closes #8. I think there is a need for a more descriptive tooltip; I can work on that while you review.
Thank you!