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

Calculate total set volume per 7 days #63

Merged
merged 5 commits into from
Dec 3, 2024

Conversation

senier
Copy link
Contributor

@senier senier commented Oct 20, 2024

I'd like to discuss a potential change to how total set volume is represented in the "Set volume (weekly total)" is represented. Currently, this calculated on a per-week basis starting on Monday. IMHO this makes the last value of the graph less helpful, as it will only be meaningful a the end of the (training) week.

What do you think about a 7 day rolling average instead? I changed the graph in this draft PR, but it could also be added in addition to the current graph. Alternatively, the set volume for he current week could be either added as a single value / horizontal line or as a value to the training overview in the main dialog (where low/optimal/high load is shown).

The same idea could also be applied to the per-muscle set volume graphs...

@treiher
Copy link
Owner

treiher commented Oct 21, 2024

I think replacing the weekly total with a rolling total over the last 7 days is a good idea. I'm actually not that interested in the already achieved set volume for the current week (i.e. the last value of the weekly total graph), so I wouldn't mind if this info will be omitted. To be consistent, the approach should be applied to all set volume graphs, and a rolling average over the last 7 days should be used for the RPE graphs.

I had a look at the current implementation and it seems to be broken. Some days have two values and some days have no value. I guess the current algorithm doesn't take into account that there could be multiple training sessions per day and doesn't calculate values for days without training sessions.

image

@senier senier force-pushed the rolling_total_set_volume branch 2 times, most recently from 0aa4b9c to 4637350 Compare November 2, 2024 20:05
@senier
Copy link
Contributor Author

senier commented Nov 2, 2024

I think replacing the weekly total with a rolling total over the last 7 days is a good idea. I'm actually not that interested in the already achieved set volume for the current week (i.e. the last value of the weekly total graph), so I wouldn't mind if this info will be omitted. To be consistent, the approach should be applied to all set volume graphs, and a rolling average over the last 7 days should be used for the RPE graphs.

I changed the set volume, volume load and RPE graphs accordingly to always include the total/average value for the training day as well as a 7 day average for the same day. Please have a look, especially at the RPE graphs as I do not have good data for this (and the demo mode looks a bit artificial to me).

I had a look at the current implementation and it seems to be broken. Some days have two values and some days have no value. I guess the current algorithm doesn't take into account that there could be multiple training sessions per day and doesn't calculate values for days without training sessions.

You are right - I fixed the algorithm to combine multiple sessions of the same day now. Do you really think it's necessary to calculate values for days without training sessions? Wouldn't the line that gets drawn between two training days be a good enough approximation? (Or maybe I'm missing something here).

The body weight graph could be refactored in a similar fashion. I didn't do that as I don't understand the rationale of the current implementation. What's the reason for the average weight being calculated as a centered average over the value from 4 days ago? Wouldn't the average over the previous 7 days be enough?

Feel free to suggest other colors.

Two additional minor changes:

  • I added an option to export the demo data to be able to use it during development
  • I changed the makefile so that an alternalve configuration can be passed from the command line (for the same reason)

@treiher
Copy link
Owner

treiher commented Nov 2, 2024

I changed the set volume, volume load and RPE graphs accordingly to always include the total/average value for the training day as well as a 7 day average for the same day. Please have a look, especially at the RPE graphs as I do not have good data for this (and the demo mode looks a bit artificial to me).

Unfortunately, the code doesn't work for me:

panicked at src/common.rs:1196:41:
attempt to subtract with overflow

I didn't find a simple solution for this issue.

You are right - I fixed the algorithm to combine multiple sessions of the same day now. Do you really think it's necessary to calculate values for days without training sessions? Wouldn't the line that gets drawn between two training days be a good enough approximation? (Or maybe I'm missing something here).

The graph can be quite inaccurate if values for days with no training sessions are not calculated. Here is a simple example: Assuming there are two training sessions with 10 sets each on Monday and Friday and no training sessions in the previous 7 days, the rolling total for the last 7 days will be as follows:

Mo 10
Tu 10
We 10
Th 10
Fr 20
Sa 20
Su 20
Mo 10

If the value for rest days is not calculated, the line would show the following values:

Mo 10
Tu 12.5
We 15
Th 17.5
Fr 20
Sa ?
Su ?
Mo ?

It would also not be nice if it did not show that the total drops after several rest days.

The body weight graph could be refactored in a similar fashion. I didn't do that as I don't understand the rationale of the current implementation. What's the reason for the average weight being calculated as a centered average over the value from 4 days ago? Wouldn't the average over the previous 7 days be enough?

A simple moving average over the previous 7 days lags behind by 3-4 days. Using a centered average avoids this problem and leads to more meaningful values in my opinion. I'm not yet sure if it makes sense to apply the same approach to RPE.

@senier senier force-pushed the rolling_total_set_volume branch from 4637350 to 7756bf0 Compare November 2, 2024 22:25
@senier
Copy link
Contributor Author

senier commented Nov 3, 2024

Unfortunately, the code doesn't work for me:

panicked at src/common.rs:1196:41:
attempt to subtract with overflow

I didn't find a simple solution for this issue.

I broke the code when refactoring it o make clippy happy and forgot to rerun the tests ;-) This is fixed now.

The graph can be quite inaccurate if values for days with no training sessions are not calculated. Here is a simple example: Assuming there are two training sessions with 10 sets each on Monday and Friday and no training sessions in the previous 7 days, the rolling total for the last 7 days will be as follows:

[...]

It would also not be nice if it did not show that the total drops after several rest days.

Thanks for the example - I get the point now and will try that approach.

A simple moving average over the previous 7 days lags behind by 3-4 days. Using a centered average avoids this problem and leads to more meaningful values in my opinion.

I don't think I understand. Why would the average of the (values available for the) last 7 days lag behind?

@treiher
Copy link
Owner

treiher commented Nov 3, 2024

Why would the average of the (values available for the) last 7 days lag behind?

A centered average results in a smoothed curve of the actual curve:

centered_average

A non-centered average gives the same curve but shifted to the right:

average

@treiher
Copy link
Owner

treiher commented Nov 3, 2024

I changed the set volume, volume load and RPE graphs accordingly to always include the total/average value for the training day as well as a 7 day average for the same day. Please have a look, especially at the RPE graphs as I do not have good data for this (and the demo mode looks a bit artificial to me).

I think it would be nicer to use a bar chart for the daily total/average and to use yellow as the second color. The result looks as follows:

bar_chart

What do you think?

Here are the corresponding code changes to the training page.
diff --git a/frontend/src/common.rs b/frontend/src/common.rs
index f3ff251..f70ef3d 100644
--- a/frontend/src/common.rs
+++ b/frontend/src/common.rs
@@ -18,9 +18,9 @@ pub const COLOR_LOAD: usize = 1;
 pub const COLOR_LONG_TERM_LOAD: usize = 2;
 pub const COLOR_LONG_TERM_LOAD_BOUNDS: usize = 13;
 pub const COLOR_RPE: usize = 0;
-pub const COLOR_RPE_7DAY: usize = 19;
+pub const COLOR_RPE_7DAY: usize = 2;
 pub const COLOR_SET_VOLUME: usize = 3;
-pub const COLOR_SET_VOLUME_7DAY: usize = 19;
+pub const COLOR_SET_VOLUME_7DAY: usize = 2;
 pub const COLOR_VOLUME_LOAD: usize = 6;
 pub const COLOR_VOLUME_LOAD_7DAY: usize = 19;
 pub const COLOR_TUT: usize = 2;
@@ -853,8 +853,8 @@ pub fn plot_bar_chart(
             .iter()
             .flat_map(|(s, _)| s.iter().map(|(_, y)| *y))
             .collect::<Vec<_>>(),
-        None,
-        None,
+        y_min_opt,
+        y_max_opt,
     );
 
     let mut result = String::new();
diff --git a/frontend/src/page/training.rs b/frontend/src/page/training.rs
index 10b5726..2484d68 100644
--- a/frontend/src/page/training.rs
+++ b/frontend/src/page/training.rs
@@ -547,14 +547,12 @@ pub fn view_charts<Ms>(
         ),
         common::view_chart(
             &[
-                ("Set volume", common::COLOR_SET_VOLUME),
+                ("Set volume (daily total)", common::COLOR_SET_VOLUME),
                 ("Set volume (7 day total)", common::COLOR_SET_VOLUME_7DAY)
             ],
-            common::plot_line_chart(
-                &[
-                    (set_volume, common::COLOR_SET_VOLUME),
-                    (set_volume_7day_total, common::COLOR_SET_VOLUME_7DAY)
-                ],
+            common::plot_bar_chart(
+                &[(set_volume, common::COLOR_SET_VOLUME),],
+                &[(set_volume_7day_total, common::COLOR_SET_VOLUME_7DAY)],
                 interval.first,
                 interval.last,
                 Some(0.),
@@ -566,9 +564,12 @@ pub fn view_charts<Ms>(
         IF![
             show_rpe =>
             common::view_chart(
-                &[("RPE", common::COLOR_RPE), ("RPE (7 day average)", common::COLOR_RPE_7DAY)],
-                common::plot_line_chart(
-                    &[(rpe, common::COLOR_RPE), (rpe_7day_avg, common::COLOR_RPE_7DAY)],
+                &[("RPE (daily average)", common::COLOR_RPE), ("RPE (7 day average)", common::COLOR_RPE_7DAY)],
+                common::plot_bar_chart(
+                    &[(rpe, common::COLOR_RPE)],
+                    &[(rpe_7day_avg, common::COLOR_RPE_7DAY)],
                     interval.first,
                     interval.last,
                     Some(5.),

@senier
Copy link
Contributor Author

senier commented Nov 3, 2024

I think it would be nicer to use a bar chart for the daily total/average and to use yellow as the second color.
What do you think?

The bar chart idea is great! I changed the plotting function to use the same domain for the y-asis as with two axes the 7-day plots are (IMHO) less useful.

I find the yellow lines rather hard to see on white background. I don't have a strong opinion, though, as I use dark mode mostly.

@senier senier force-pushed the rolling_total_set_volume branch 2 times, most recently from f219906 to 3a4cd35 Compare November 6, 2024 21:50
@senier
Copy link
Contributor Author

senier commented Nov 6, 2024

The idea is implemented now - please have a look.

There are a few things that I'd still like to look into:

  1. Try the same bar/line chart combination for body weight
  2. Do not draw dots in the line chart in places where there is no measurement (might look a bit smoother)
  3. Omit the line in line charts between 2 values that are zero (this would avoid long lines at the bottom of the graph where no data is available)

@treiher
Copy link
Owner

treiher commented Nov 7, 2024

On the training page using a 7-day rolling total/average looks good to me. I'm not sure how useful the bars with the daily average/daily total actually are. Especially in the RPE chart I have the feeling that the bars for the daily average don't add meaningful information. So I would suggest to at least remove them in the RPE chart.

Threre is also bug in the RPE chart. If there are two training sessions in one day, the RPE values are summed instead of averaged:

image

For some reason, this also results in too high values for the 7-day average for the entire interval displayed:

image


I'm not convinced about using this new representation for exercises (and also routines). Here is the new representation:

image

In my opinion the old representation looks better and trends are easier to see:

image

Again, the bars in the new representation do not seem very helpful to me. So I would suggest to remove them here. I'm undecided whether we should use a weekly average/total as before or a rolling 7-day average.

Looking at both representations, I think there is also a bug in the old representation. The line should go to zero if there hasn't been a training session in a week, which isn't the case.


I find the yellow lines rather hard to see on white background. I don't have a strong opinion, though, as I use dark mode mostly.

I agree, yellow isn't optimal. We could try instead using different colors for the lines (as in the old representation) and grey for the bars (in case we want to keep them).

@senier
Copy link
Contributor Author

senier commented Nov 10, 2024

On the training page using a 7-day rolling total/average looks good to me. I'm not sure how useful the bars with the daily average/daily total actually are. Especially in the RPE chart I have the feeling that the bars for the daily average don't add meaningful information. So I would suggest to at least remove them in the RPE chart.

I must say, looking at real data the bar chars look awful. We should indeed try something else. Another option is to represent single values as dots and the calculated averages/totals as a lines along with it (similarly to how you would plot statistical data):

image
image
image

Please also have a look at the body weight, for which I tried the same graph layout.

Don't look at the code too closely, though, as I just quickly hacked it. I definitely must clean up the mess later. Some of the other graphs will also be broken due to the change. Some colors will also be off elsewhere.

Threre is also bug in the RPE chart. If there are two training sessions in one day, the RPE values are summed instead of averaged:

That was an error on my part - I just used the wrong function (RPE values got added instead of averaged). This should be fixed now - please have a look.

For some reason, this also results in too high values for the 7-day average for the entire interval displayed:

That might be for the same reason - can you please check this again? My values looked good.

I'm not convinced about using this new representation for exercises (and also routines). Here is the new representation:
[...]
In my opinion the old representation looks better and trends are easier to see:
[...]
Again, the bars in the new representation do not seem very helpful to me. So I would suggest to remove them here

Yep. Let me know what your feeling is about the current suggestions. For exercises I find it pretty helpful now (at least for my real world data). Not sure about the set volumes in routines - my routines usually are 7 days apart such that the 7 day total matches the single values...

I'm undecided whether we should use a weekly average/total as before or a rolling 7-day average.

For all charts or just the exercises? In general, I have my feeling is that the 7 day average looks a bit smoother.

Looking at both representations, I think there is also a bug in the old representation. The line should go to zero if there hasn't been a training session in a week, which isn't the case.

I believe I fixed that. My most recent change makes the y-values of type Option<f32> and plots line series in chunks if they are interspersed by None:

image

I agree, yellow isn't optimal. We could try instead using different colors for the lines (as in the old representation) and grey for the bars (in case we want to keep them).

That's what I tried (for the dots).

@treiher
Copy link
Owner

treiher commented Nov 10, 2024

I must say, looking at real data the bar chars look awful. We should indeed try something else. Another option is to represent single values as dots and the calculated averages/totals as a lines along with it (similarly to how you would plot statistical data):

image

The dots look much better than the bars! I still don't see the value in showing the daily total/average in the charts. So I would rather remove these values for the set volume, volume load and RPE charts. If you disagree, it would be okay to me to keep them, as the dots don't detract from the actual interesting information.

In my opinion using dots for the short-term load makes the chart worse. Especially for longer intervals it's just a cloud of dots and the actual course of the short-term load isn't visible. Using dots for the long-term load might work better, but I guess a line would still be better. I would also prefer to continue using green for the short-term load.

Please also have a look at the body weight, for which I tried the same graph layout.

image

I like that the long term changes are better visible in the new layout. Having no dots on the line for the average also looks better. I see the drawback that the actual short-term changes are less visible, which at least one other user I've just spoken to doesn't think is so great. So I'm slightly in favor of keeping to use a line instead of dots.

image

For the body weight chart, I would prefer if there were no gaps if there was no value in 7 days. I think it's not that uncommon for a user to miss tracking their weight for a week.

Previously there was a small padding on the y-axis to prevent dots from being drawn directly on the axis. Has this been removed on purpose?

Threre is also bug in the RPE chart. If there are two training sessions in one day, the RPE values are summed instead of averaged:

That was an error on my part - I just used the wrong function (RPE values got added instead of averaged). This should be fixed now - please have a look.

For some reason, this also results in too high values for the 7-day average for the entire interval displayed:

That might be for the same reason - can you please check this again? My values looked good.

Yes, this looks good now. 👍

Yep. Let me know what your feeling is about the current suggestions. For exercises I find it pretty helpful now (at least for my real world data). Not sure about the set volumes in routines - my routines usually are 7 days apart such that the 7 day total matches the single values...

As I typically only do an exercise once a week, the dots with the daily total/average are completely useless in my case:

image

There are also a lot of gaps, which don't look very nice. Probably because there is some variance in my training days and thus I don't do the same exercise exactly every 7 days. For routines there is the same problem.

I'm undecided whether we should use a weekly average/total as before or a rolling 7-day average.

For all charts or just the exercises? In general, I have my feeling is that the 7 day average looks a bit smoother.

The 7-day average/total looks good on the training page. The lines are smoother than before. While the new representation works well on the training page, because there is usually sufficient data, it seems not optimal to me on the exercise and routine page. There are a lot of steps and gaps, as there is often only one data point or no data in a 7-day interval (as in my example shown above).

I believe I fixed that. My most recent change makes the y-values of type Option<f32> and plots line series in chunks if they are interspersed by None:

image

I think it depends on the type of data whether there should be a gap or a line at 0. If it is an average like for RPE, dropping to 0 wouldn't make sense. The average RPE isn't 0 if you didn't log any set with an RPE. For totals like set volume, having a line at 0 would make sense. If you didn't do any sets, the set volume for this interval is 0.


I think it's great that you're trying to find better representations. I hope my complaints don't discourage you.

@senier
Copy link
Contributor Author

senier commented Nov 13, 2024

Thanks for the feedback. As the discussion got a bit longer, here is a summary of what I'm planning to implement now:

  • Plots

    • Plot averages without gaps in intervals
    • Plot zero values where no data is available
    • Use gray dots and colored average/total (where applicable)
    • Check the padding issue to avoid overlap with x axis
  • Body weight

    • Use new 7 day average calculation for graph (but no gaps)
    • Use a line without dots for average weight
    • Keep using a line for short term weight data (with or without dots - to be seen)
  • Training page

    • Keep long-/short-term load as it was before
    • Display set volume as 7 day total (no daily)
    • Display RPE as 7 day average (no daily)
  • Routines page

    • Keep charts as they were before
  • Exercises page

    • Keep set volume chart as before
    • Keep RPE chart as before
    • Volume load: see discussion below
  • Muscles page

    • Plot daily and 7 day average

Can you see anything I've dropped or misunderstood from the previous discussion?

Per-exercise volume load

Having an average here in addition to the daily entries has indeed value for me to better identify a trend. I do a lot of undulating exercises which make the single value graph rather useless for that purpose. I just tried an average over 31 days and that produces a pretty helpful trend in my case. Do you think that would be worth trying? I could also imagine scaling the averaging window depending on the interval shown (e.g. don't show averages for 1M, show 15 day average for 3M, show 31 day average for 6M ...).

@treiher
Copy link
Owner

treiher commented Nov 13, 2024

  • Plots

    • Plot averages without gaps in intervals
    • Plot zero values where no data is available

For the 7 day calculations I would rather say:

  • Plot averages with gaps if no data is available
  • Plot totals as zero if no data is available
  • Body weight

    • Use new 7 day average calculation for graph (but no gaps)

I think the 7-day average calculation doesn't make sense for the body weight chart. If there is only one entry per week, or even less, there will be no average. It's not uncommon to only weigh once a week.

image

The current algorithm adapts to the interval between weight measurements as it always looks at 9 consecutive entries. This is probably the better approach to cover different usage patterns.

  • Muscles page

    • Plot daily and 7 day average

Do you see an advantage of plotting the daily set volume here? This would be inconsistent with what is done on the training page as per your point above.

Exercise page

  • Keep set volume chart as before
  • Volume load: see discussion below

Per-exercise volume load

Having an average here in addition to the daily entries has indeed value for me to better identify a trend. I do a lot of undulating exercises which make the single value graph rather useless for that purpose. I just tried an average over 31 days and that produces a pretty helpful trend in my case. Do you think that would be worth trying? I could also imagine scaling the averaging window depending on the interval shown (e.g. don't show averages for 1M, show 15 day average for 3M, show 31 day average for 6M ...).

I think making the average dependend on the shown time interval doesn't solve the problem. The problem of the fixed X-day average is that the resulting average is only useful if there is more than one data point per X days. We could try to solve the problem by using the same approach that is currently used for the body weight average, which is to compute a centered rolling average of a fixed number of values. What do you think?

@senier senier mentioned this pull request Nov 20, 2024
@senier senier force-pushed the rolling_total_set_volume branch from 1d1bba5 to ac7d9ab Compare November 21, 2024 22:00
@senier
Copy link
Contributor Author

senier commented Nov 21, 2024

* Plot averages with gaps if no data is available

I could add this. IMHO having a straight line instead of a gap looks OK, too. What do you think?

* Plot totals as zero if no data is available

That's what I meant to say and what I implemented.

I think the 7-day average calculation doesn't make sense for the body weight chart. If there is only one entry per week, or even less, there will be no average. It's not uncommon to only weigh once a week.
[...]
The current algorithm adapts to the interval between weight measurements as it always looks at 9 consecutive entries. This is probably the better approach to cover different usage patterns.

I'm not sure how much sense it makes to include a weight that was recorded a couple of months ago (if records are only that infrequent) into the average calculation. The same is true for the per exercise volume load. There should probably be some cap on how old values can be (relative to the center) to be included into the average calculation to make sense. I'd leave this for another discussion and not change either of those charts in this PR.

  • Muscles page

    • Plot daily and 7 day average

Do you see an advantage of plotting the daily set volume here? This would be inconsistent with what is done on the training page as per your point above.

No, that's not useful. I removed that.

@senier
Copy link
Contributor Author

senier commented Nov 21, 2024

@treiher You can also have a look at this branch if you like and let me know what you think. It uses area plots for the total set volume charts. IMHO that's an interesting option.

@treiher
Copy link
Owner

treiher commented Nov 22, 2024

* Plot averages with gaps if no data is available

I could add this. IMHO having a straight line instead of a gap looks OK, too. What do you think?

I think a straight line is also fine. I just have the feeling that a gap would be a better representation of "no data" in that case. Would it be difficult to add this so I can see how it looks with my data?

* Plot totals as zero if no data is available

That's what I meant to say and what I implemented.

With the current version there is a straight line, although the total should be zero:

image

I think the 7-day average calculation doesn't make sense for the body weight chart. If there is only one entry per week, or even less, there will be no average. It's not uncommon to only weigh once a week.
[...]
The current algorithm adapts to the interval between weight measurements as it always looks at 9 consecutive entries. This is probably the better approach to cover different usage patterns.

I'm not sure how much sense it makes to include a weight that was recorded a couple of months ago (if records are only that infrequent) into the average calculation. The same is true for the per exercise volume load. There should probably be some cap on how old values can be (relative to the center) to be included into the average calculation to make sense. I'd leave this for another discussion and not change either of those charts in this PR.

You are probably right, an upper limit for unreasonably long intervals between values seems reasonable.

The current plan is to just change the set volume and RPE charts on the training and muscle page in this PR, right? For consistency, I would suggest to also remove the dots in the short-term/long-term load chart on the training page.

@treiher You can also have a look at this branch if you like and let me know what you think. It uses area plots for the total set volume charts. IMHO that's an interesting option.

That's indeed a nice alternative. This could be also a good option for several other charts. Using it only for the total set volume charts seems a bit arbitrary to me.

I think it would be also nice to have some kind of an area plot for the long-term load chart:

image

Do you think that would be difficult to implement?

@senier
Copy link
Contributor Author

senier commented Nov 23, 2024

That's indeed a nice alternative. This could be also a good option for several other charts. Using it only for the total set volume charts seems a bit arbitrary to me.

Sure, we could think about using it elsewhere, too.

Do you think that would be difficult to implement?

It's almost possible out of the box:

image

This is in fact a hack. As area plots always start at the y=0, I first plot the upper graph and then the lower graph in background color on top of it. Not great, as you lose the grid and the background is not a perfect match (more hacks would be needed here) and yet another hack would be needed to support light/dark mode.

The better solution is probably to implement a custom band graph with plotters Polygon drawing functionality. I'll look into that at some point as I plan to use it for nutrition tracking.

@treiher
Copy link
Owner

treiher commented Nov 23, 2024

This is in fact a hack. As area plots always start at the y=0, I first plot the upper graph and then the lower graph in background color on top of it. Not great, as you lose the grid and the background is not a perfect match (more hacks would be needed here) and yet another hack would be needed to support light/dark mode.

The better solution is probably to implement a custom band graph with plotters Polygon drawing functionality. I'll look into that at some point as I plan to use it for nutrition tracking.

Then it's better to keep using lines until we have a custom implementation.

@senier senier force-pushed the rolling_total_set_volume branch from ac7d9ab to 853b327 Compare November 23, 2024 23:00
@senier
Copy link
Contributor Author

senier commented Nov 23, 2024

I think a straight line is also fine. I just have the feeling that a gap would be a better representation of "no data" in that case. Would it be difficult to add this so I can see how it looks with my data?

Implementing this is not that hard. Have a look at this branch.

With the current version there is a straight line, although the total should be zero:

image

The total volume graph was totally wrong as it used the average calculation 🤦 This should work as expected now, please have a look.

The current plan is to just change the set volume and RPE charts on the training and muscle page in this PR, right?

Correct.

For consistency, I would suggest to also remove the dots in the short-term/long-term load chart on the training page.

Done.

@senier senier marked this pull request as ready for review November 23, 2024 23:07
@treiher
Copy link
Owner

treiher commented Nov 24, 2024

I think a straight line is also fine. I just have the feeling that a gap would be a better representation of "no data" in that case. Would it be difficult to add this so I can see how it looks with my data?

Implementing this is not that hard. Have a look at this branch.

Looks good to me. I would prefer this solution.

The total volume graph was totally wrong as it used the average calculation 🤦 This should work as expected now, please have a look.

Now it looks correct.

For consistency, I would suggest to also remove the dots in the short-term/long-term load chart on the training page.

Done.

👍

The only remaining issue I see is the one mentioned in #64 (comment). I will have another look at the code after #64 is merged.

@senier senier force-pushed the rolling_total_set_volume branch from 853b327 to 725c02b Compare November 24, 2024 14:46
@senier
Copy link
Contributor Author

senier commented Nov 24, 2024

I included the gap-related change and rebased this branch onto #64.

Btw, I also rebased and updated the area plot branch to improve it visually and apply it to all volume charts. Let me know what you think and whether we want to include it in this PR.

frontend/src/ui/page/training.rs Outdated Show resolved Hide resolved
frontend/src/ui/common.rs Show resolved Hide resolved
frontend/src/ui/common.rs Show resolved Hide resolved
frontend/src/ui/common.rs Outdated Show resolved Hide resolved
frontend/src/ui/common.rs Outdated Show resolved Hide resolved
@treiher
Copy link
Owner

treiher commented Nov 24, 2024

Btw, I also rebased and updated the area plot branch to improve it visually and apply it to all volume charts. Let me know what you think and whether we want to include it in this PR.

Looks good! I just think that we should use the same presentation also for other charts to get a consistent look. It could probably be applied to all other training-related charts, except the short-term/long-term load chart.

I would suggest creating a separate PR for this change so that this PR is not delayed any further.

@senier senier force-pushed the rolling_total_set_volume branch from 725c02b to 8d4c695 Compare December 1, 2024 16:13
@senier senier requested a review from treiher December 1, 2024 16:16
frontend/src/ui/page/training.rs Outdated Show resolved Hide resolved
frontend/src/ui/page/training.rs Outdated Show resolved Hide resolved
frontend/src/ui/page/muscles.rs Outdated Show resolved Hide resolved
frontend/src/ui/page/muscles.rs Outdated Show resolved Hide resolved
frontend/src/ui/common.rs Show resolved Hide resolved
@senier senier force-pushed the rolling_total_set_volume branch from 8b86b87 to 561bebe Compare December 2, 2024 22:33
@senier senier force-pushed the rolling_total_set_volume branch from 561bebe to 6fa81b7 Compare December 2, 2024 22:37
@senier senier requested a review from treiher December 2, 2024 22:41
frontend/src/ui/common.rs Outdated Show resolved Hide resolved
@treiher treiher merged commit 02b5c4b into treiher:main Dec 3, 2024
26 checks passed
@senier senier deleted the rolling_total_set_volume branch December 3, 2024 21:42
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.

2 participants