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

fix: simplify and ensure correctness of data aggregation #112

Merged

Conversation

muktihari
Copy link
Member

Instead of manually aggregating Lap and Session structs per field basis where the number of fields to be accommodated is unlimited, let's use reflection to do the job, this way we don't lose any data and our code become simpler. The aggregator code is copied from https://github.com/muktihari/fit/blob/master/cmd/fitactivity/aggregator/aggregator.go with some modifications to accommodate our needs.

The use of reflection is minimal as the number of laps and sessions to aggregate is small, it's fast enough to run in WebAssembly.

@muktihari muktihari added bug Something isn't working enhancement New feature or request labels Sep 8, 2024
@muktihari muktihari self-assigned this Sep 8, 2024
@raditzlawliet
Copy link
Member

Is there an example of the problem case and the solved result with this PR?

@muktihari
Copy link
Member Author

muktihari commented Sep 8, 2024

Is there an example of the problem case and the solved result with this PR?

Please take a look at the structure of the session message first:
https://github.com/muktihari/fit/blob/master/profile/mesgdef/session_gen.go.

There are so many fields that need to be aggregated, previously we were only calculateing around 22 out of 155 fields (14%). While not all fields will be present since some fields are sport specific, we are certainly missing some fields.

For instance, some manufacturers such as Garmin prefer using fields start with "Enhanced" prefix (EnhancedAvgSpeed, EnhancedMaxSpeed, EnhancedAvgAltitude, etc). We were not able to aggregate those field, resulting data loss.

If we keep the previous method to manually declare aggregation, just how many code that we need to add those fields, not only Session, we also need to aggregate Lap as well that has similar number of fields

@muktihari
Copy link
Member Author

muktihari commented Sep 8, 2024

This PR related to #113 . This PR handle the data aggregation (Lap and Session), where #113 handles preprocessing Record and data presentation (marshaling into json that will be consumed by FE). If you want file to test, you can use https://github.com/roznet/fit-benchmarks/blob/main/large.fit.

You can also use fitconv to convert the file into csv file, so you can see what's in the file.

@muktihari
Copy link
Member Author

I just share you a gdrive folder containing sample FIT files that you can test ya @raditzlawliet

@muktihari
Copy link
Member Author

I bump the sdk to v0.21.5 to address this issue: muktihari/fit#397

@muktihari
Copy link
Member Author

Close due to #114

@muktihari muktihari reopened this Sep 23, 2024
@muktihari
Copy link
Member Author

Reopened due #114 is postponed

@muktihari muktihari merged commit 801454e into master Sep 23, 2024
2 checks passed
@muktihari muktihari deleted the fix/simplify-and-ensure-correctness-of-data-aggregation branch September 23, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants