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

Ignore Invalid HR on HR Zone #68

Merged
merged 7 commits into from
Dec 5, 2023

Conversation

raditzlawliet
Copy link
Member

@raditzlawliet raditzlawliet commented Dec 4, 2023

  • Compare record with valid HR and seek next valid record till end of records
  • Adjust percentage to ignore HR value outside of zone (e.g., Zone 1 will have 95-105, all HR <95 will ignored in time & percentage dispaly)
  • Adjust on collapse HR to same with other module
  • Optimize finding HR Zone with local cache

for (let i = 0; i < session.records.length - 1; i++) {
const entry = session.records[i]
const nextEntry = session.records[i + 1]

const hrZoneIndex = this.getHeartRateZoneIndex(entry.heartRate || 0)
const nextHrZoneIndex = this.getHeartRateZoneIndex(nextEntry.heartRate || 0)
if (entry.heartRate == null || nextEntry.heartRate == null) continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have these records pattern, do we still get zone? It seems like will be skipped entirely, no? What if for nextEntry instead of [i+1], we try find up to next record with valid heartRate? Then we move our i cursor to that record index.

records[0].heartRate = 120
records[1].heartRate = null
records[2].heartRate = 130
records[3].heartRate = null
records[4].heartRate = 135
records[5].heartRate = null

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will skipped entirely.
is it possible to have that case commonly?
if such case exists as rare, we can adjust how will we recognize this data case.

What if for nextEntry instead of [i+1], we try find up to next record with valid heartRate?

If this process is valid, we can adjust to it.

Assume there a case

records[0].heartRate = 120
records[1].heartRate = null
records[2].heartRate = null
records[3].heartRate = null
records[4].heartRate = null
records[5].heartRate = 135

what should we do then? compare 120 and 135?

Copy link
Member

@muktihari muktihari Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to have that case commonly?

We can't be sure since the data is from sensor, sometimes the sensor is not working properly due to low battery or position, network fail between sensor to cyclocomp, etc.

what should we do then? compare 120 and 135?

Yes, compare let's compare 120 to 135, and calculate the elapse between them as usual.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, latest commit now will do compare 120 and 135.
then next record will do 135 and next valid HR (if no valid HR, it will compare with themself for diff time 1s)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since i don't have invalid data, i'll do test manually via code. kindly help me check with your real data

hr zone, now seeking next HR Record for comparator (till end of record). if no record valid, will use current record as comparator and end the calculation.
hr zone, fix percentage time on not-included hr zone (e.g., HeartRate under minimal visibility zone)
Comment on lines 366 to 371
for (let i = 0; i < session.records.length - 1; i++) {
const entry = session.records[i]
const nextEntry = session.records[i + 1]
if (entry == null || entry.heartRate == null) continue

if (entry.heartRate == null || nextEntry.heartRate == null) continue
let { nextEntry, nextIndex } = this.getNextValidEntry(session, entry, i)
i = nextIndex // skip loop to latest valid entry
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i = nextIndex, next loop i will be +1 since i++

records[0] -> entry
records[1] -> nextEntry
records[2] -> next loop entry will be this  ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it should be i = nextIndex -1

for (let i = 0; i < session.records.length - 1; i++) {
const entry = session.records[i]
const nextEntry = session.records[i + 1]
if (entry == null || entry.heartRate == null) continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually entry will never be null, it's guaranteed. but this is okay too, no worries.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually entry will never be null, it's guaranteed. but this is okay too, no worries.

i think this leftover from previous one that Record can be null (cause linter red), i'll check more on this

: Math.min(
Math.max(this.hrZones[zoneIndex].minmax[0], nextEntry.heartRate ?? 0),
this.hrZones[zoneIndex].minmax[1]
)
return 1 + total + (Math.max(end, start) - Math.min(end, start))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be simplified though

return 1 + total + Math.abs(start-end)

btw one question pls, if zoneIndex == -1 what will happen then? Can you give a little illustration regarding this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entry.heartRate = 50
nextEntry.heartRate = 200

assume 50 is below zone 1, and 200 is above zone 5, it will belong to the zone who has (200-50) 150bpm in it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe more real world case:

entry.heartRate = 50 (assume not belong to zone)
entry.heartRate = 90 (assume zone 1)

it will add 40 steps then distribute them evenly based on elapsed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entry.heartRate = 50
nextEntry.heartRate = 200

It will do transition 50 (inclusive) to 200 (inclusive) with each value added time to zone (1/151 * deltaTime)
roughly figure like this

200 BPM = 1/151 * deltaTime -> Zone 5
...
50 BPM = 1/151 * deltaTime -> No Zone

because Zone 5 is 90% to Infinity, therefore 200+ still counted as Zone 5.
because Zone 1 is 50%-60%, therefore 50 didn't counted as Zone 1 and discarded.

… loop.

remove record check null (from previous method calculation).
remove some log
Copy link
Member

@muktihari muktihari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

optimize finding hr zone using temporary local cache to efficiency finding hr zone on large records
@raditzlawliet
Copy link
Member Author

Minor optimize performance

@raditzlawliet raditzlawliet merged commit 520178c into dev Dec 5, 2023
1 check passed
@raditzlawliet raditzlawliet deleted the improvement/hr-zone-ignore-invalid-hr branch December 7, 2023 08:13
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