Calculate percentage spent in each HR zone

[khink]

I was looking for a simple way to extract the percentage of workout spent in each HR zone from .tcx files, and this module seems the best basis.

This calculates the percentage based on number of measurements (TrackPoints), it assumes the time between these measurements is roughly equal. This is not the most precise way, to do it, but for my purpose (and, i would say, arguably for most) it suffices, as usually the measurements are taken in regular intervals.

I hope you find it useful as well.

[vkurup]

Thanks for taking the time to contribute this! I don't personally use this library anymore, but this looks really useful. I made a few comments, which I'll try to get to at some point if you don't beat me to it.

[vkurup]

Suggested change

	"""Percentage of workout spent in eacht heart rate zone.
	"""Percentage of workout spent in each heart rate zone.

[vkurup]

Suggested change

	Given these user's heart rate zones:
	{
	"Z0": (0, 119),
	"Z1": (120, 199),
	"Z2": (200, 240),
	}
	Given these user's heart rate zones:
	zones = {
	"Z0": (0, 119),
	"Z1": (120, 199),
	"Z2": (200, 240),
	}

[vkurup]

Suggested change

	We return something like:
	{
	"Z0": 5,
	"Z1": 95,
	"Z2": 0,
	}
	Then `self.hr_percent_in_zones(zones)` would return something like:
	{
	"Z0": 5,
	"Z1": 95,
	"Z2": 0,
	}

[vkurup]

I now use Python's cool defaultdict for this. It's a dictionary that acts like a normal dictionary unless you try to assign values to an unknown key, in which case, it will create an entry for that key, defaulting to whatever you'd like it to default to. So this line could be replaced with:

from collections import defaultdict
per_zone = defaultdict(int)  # <- int class defaults to zero

[khink]

This code could indeed be more elegant. I found defaultdict by itself does not do the main thing that i want to happen here, which is create a dictionary with same keys as zones. I propose to change it to this:

        # Initialize a dictionary with one item for each zone
        per_zone = dict.fromkeys(zones.keys(), 0)

[vkurup]

Ahh good point. This looks great.

[vkurup]

I'm not 100% sure, but I think it would be possible for nr_hr_values to be zero so we'd get a division by zero error here (like if a user called this method without having any valid HR data parsed in their data file). So we should probably just make sure that nr_hr_values is not zero. Better safe than sorry :)

[khink]

I agree. As errors should not go unnoticed, i decided to raise a NoHeartRateDataError in this case.

[khink]

Thanks for your response! Some of your comments already make a lot of sense to me. Expect an update soon.

…

On Sat, 19 Dec 2020, 20:37 Vinod Kurup, ***@***.***> wrote: ***@***.**** commented on this pull request. Thanks for taking the time to contribute this! I don't personally use this library anymore, but this looks really useful. I made a few comments, which I'll try to get to at some point if you don't beat me to it. ------------------------------ In tcxparser/tcxparser.py <#29 (comment)> : > @@ -98,6 +98,39 @@ def hr_min(self): """Minimum heart rate of the workout""" return min(self.hr_values()) + def hr_percent_in_zones(self, zones): + """Percentage of workout spent in eacht heart rate zone. ⬇️ Suggested change - """Percentage of workout spent in eacht heart rate zone. + """Percentage of workout spent in each heart rate zone. ------------------------------ In tcxparser/tcxparser.py <#29 (comment)> : > + Given these user's heart rate zones: + { + "Z0": (0, 119), + "Z1": (120, 199), + "Z2": (200, 240), + } ⬇️ Suggested change - Given these user's heart rate zones: - { - "Z0": (0, 119), - "Z1": (120, 199), - "Z2": (200, 240), - } + Given these user's heart rate zones: + + zones = { + "Z0": (0, 119), + "Z1": (120, 199), + "Z2": (200, 240), + } ------------------------------ In tcxparser/tcxparser.py <#29 (comment)> : > + We return something like: + { + "Z0": 5, + "Z1": 95, + "Z2": 0, + } ⬇️ Suggested change - We return something like: - { - "Z0": 5, - "Z1": 95, - "Z2": 0, - } + Then `self.hr_percent_in_zones(zones)` would return something like: + { + "Z0": 5, + "Z1": 95, + "Z2": 0, + } ------------------------------ In tcxparser/tcxparser.py <#29 (comment)> : > + "Z0": (0, 119), + "Z1": (120, 199), + "Z2": (200, 240), + } + + We return something like: + { + "Z0": 5, + "Z1": 95, + "Z2": 0, + } + + Correct calculation relies on evenly spaced measurement times. + """ + # count number of HR measurements per zone + per_zone = dict(zip(zones.keys(), [0 for x in zones.keys()])) I now use Python's cool defaultdict for this. It's a dictionary that acts like a normal dictionary unless you try to assign values to an unknown key, in which case, it will create an entry for that key, defaulting to whatever you'd like it to default to. So this line could be replaced with: from collections import defaultdict per_zone = defaultdict(int) # <- int class defaults to zero ------------------------------ In tcxparser/tcxparser.py <#29 (comment)> : > + } + + Correct calculation relies on evenly spaced measurement times. + """ + # count number of HR measurements per zone + per_zone = dict(zip(zones.keys(), [0 for x in zones.keys()])) + hr_values = self.hr_values() + for hr in hr_values: + for zone_name, zone_boundaries in zones.items(): + if hr >= zone_boundaries[0] and hr <= zone_boundaries[1]: + per_zone[zone_name] += 1 + + # convert counts to percentages + nr_hr_values = len(hr_values) + for name, count in per_zone.items(): + per_zone[name] = round(100 * count / nr_hr_values) I'm not 100% sure, but I think it would be possible for nr_hr_values to be zero so we'd get a division by zero error here (like if a user called this method without having any valid HR data parsed in their data file). So we should probably just make sure that nr_hr_values is not zero. Better safe than sorry :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#29 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABL3OWDLW3YE4RDOGL7HQ3SVT6HVANCNFSM4UJUYX4A> .

[khink]

I see now that the tests failed for Python2.7, because of the super().__init__(self.message) construct.

Do you see any reason to keep supporting Python 2? It has been officially EOL since the beginning of 2020. In my PR, i've "solved the problem" by removing Python 2.7 from the test matrix.

[vkurup]

Agree that there's no need to support 2.7 any longer. Thank you for this contribution.

[khink]

Thanks for the review!