search

asiripanich / emdash

An e-mission deployer's dashboard. See https://github.com/e-mission/e-mission-docs.
https://emdash.amarin.dev
Other
6 stars 3 forks source link

Fix summarise trips without trips #54

Closed allenmichael099 closed 3 years ago

allenmichael099 commented 3 years ago

@asiripanich Shankari and I noticed today that there was an error with some of the trip summary columns. All participants had the same value. Also, the participation period plot was uniform. After looking into it, I realized that the mistake was in how I implemented summarise_trips_without_trips. It was using the min and max dates for all trips, rather than the min and max for each user.

It seems to be fixed but perhaps it should be more carefully tested. I threw together the beginnings of a test that for it in test-utils_tidy_data, and the columns are mostly the same. One version seems to round the timestamps to the nearest day while the other does not. n_active_days might appear in a different order.

Here are relevant screenshots of the issue (see last trip date and number of days since app install): Wrong_data_upload Wrong_particip_pd

And after the fix: Fixed_data_upload Fixed_Particip_Pd

asiripanich commented 3 years ago

@allenmichael099 just wondering why did you comment that test out? c821219

allenmichael099 commented 3 years ago

@asiripanich I figured it wasn't ready yet, and I didn't know what expectation to set.

asiripanich commented 3 years ago

@allenmichael099 Could you please submit another PR with unit tests, after Aug is fine. Thanks!

asiripanich commented 3 years ago

@allenmichael099 Now that we have summarise_trips_without_trips(), do we still need summarise_trips()

allenmichael099 commented 3 years ago

@asiripanich I kept summarise_trips to test whether the output of summarise_trips_without_trips is different than it should be. It might still be useful to have summarise_trips to compare with the output from summarise_trips_without_trips after any future changes to summarise_trips_without_trips.

asiripanich commented 3 years ago

@asiripanich I kept summarise_trips to test whether the output of summarise_trips_without_trips is different than it should be. It might still be useful to have summarise_trips to compare with the output from summarise_trips_without_trips after any future changes to summarise_trips_without_trips.

That is what I thought you were trying to do. Instead of that, we can use testthat to take a snapshot of the output and test against that. See https://github.com/asiripanich/emdash/blob/master/tests/testthat/_snaps/utils_tidy_data.md.