🧹 📊 Metrics cleanup #1111

Abby-Wheelis · 2023-12-04T17:51:36Z

Addressing code review from #1098 and codecov results from the same PR

from review, adding the calculations in comments with the tests helps to make the arithmetic more credible e-mission#1098 (review)

adding specific numbers from fakeLabels.json for the footprints, but keeping the testing at just the keys for mets due to complexity the keys in the fallback are different, so we can be sure here that the custom mets were accessed

codecov · 2023-12-04T18:00:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9ddcf31) 77.55% compared to head (6bfef79) 78.45%.
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1111      +/-   ##
==========================================
+ Coverage   77.55%   78.45%   +0.90%     
==========================================
  Files          28       28              
  Lines        1702     1699       -3     
  Branches      367      364       -3     
==========================================
+ Hits         1320     1333      +13     
+ Misses        382      366      -16

Flag	Coverage Δ
unit	`78.45% <100.00%> (+0.90%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
www/js/metrics/customMetricsHelper.ts	`95.91% <100.00%> (+17.91%)`	⬆️
www/js/metrics/footprintHelper.ts	`94.11% <ø> (+8.00%)`	⬆️

... and 1 file with indirect coverage changes

checking the 'ON_FOOT' case verifying negative speed returns 0, even if there is a fallback

Abby-Wheelis · 2023-12-04T21:45:52Z

I made a discovery working through upping the codecov report - in both metHelper and footprintHelper we have some code that catches if the mode is 'ON_FOOT' and sets it to WALKING --
in metHelper this is great -- we have the met-equivalents and fallbacks, so it turns out the same as 'WALKING but in 'footprintHelper -- we are now always using custom modes, so we have things like walk and it does not recognize WALKING as a part of the list

For footprintHelper -- if the mode is ON_FOOT -- that means the energy value will always be 0, would it be bad to hardcode a return of 0 if the mode is ON_FOOT

Also, is there anywhere we still use ON_FOOT? I'm guessing this came from some previous iteration, as I don't think I've seen it in play as a base mode or label, is it still lurking somewhere? I suppose removing the code and allowing ON_FOOT to go to the default value is also an option.

Abby-Wheelis · 2023-12-04T22:10:00Z

For footprintHelper -- if the mode is ON_FOOT -- that means the energy value will always be 0, would it be bad to hardcode a return of 0 if the mode is ON_FOOT

This is actually even more complicated when it comes to the IN_VEHICLE tests... since we don't know what all the vehicle modes are...
else if (mode == 'IN_VEHICLE') { const sum = footprint['CAR'] + footprint['BUS'] + footprint['LIGHT_RAIL'] + footprint['TRAIN'] + footprint['TRAM'] + footprint['SUBWAY']; result += (sum / 6) * mtokm(userMetrics[i].values); }

What do we want to do there? My first instinct would be to cache some sort of calculation of all the modes with IN_VEHCILE as a met-equivalent, but even that starts mixing the mets into the carbon calculations and could be overturned as we add user-custom labels, grid data, etc

added explicit throw of error and test to detect the thrown error, calculations now in try/catch block to account for errors also now allowing "ON FOOT" to go to default pending decision about "IN VEHICLE" for final coverage gap

also renamed the clearing function to ensure it was clear it was only for testing

Abby-Wheelis · 2023-12-04T22:46:06Z

Something else I've found while looking into the code coverage: the range - limits for modes
I can see where this WAS used in the code, but I don't think it's used anymore. I believe this was something that might have influenced things like mode recommendations, but we don't currently have those, should I just take it out? I could also invent some labels just for the tests to test it, but I don't think any of the labels in production have this, and the code base doesn't have the feature.

test was failing because error was not thrown, it was handled, throwing error, which will be caught by the code that displays the metrics

Abby-Wheelis · 2023-12-04T23:07:35Z

The few uncovered sections in the metrics files are now pending discussion of the above issues:

difference between default and fallback keys in the footprint calculations: how to handle ON_FOOT and IN_VEHICLE
range_limit_km - I believe this is a holdover from a previous feature (recommendations?) as we don't seem to use it, so the best course of action is probably to remove it for now, though I think I see a relatively easy path to get it covered by tests if we decide we want that.

Once we decide those two things, I think I can get the last few patches of lines tested.

After we talked about it in our meeting today, I'm going to go ahead and take this out, since we now have custom modes, really, either it's in the list or it is not, so we can't do this type of fallback anymore (at least not right now) it could be something to think about as we overhaul the metrics in upcoming months

This was old code, and no longer used elected to take it out rather than test it, this concept is something we should keep in mind if we integrate suggestions back into the app (ie I can't e-bike from CO to GA, no matter how much emissions it would save)

this label has no co2 or met information, which hits some of the edge cases in the custom metrics code

Abby-Wheelis · 2023-12-05T21:35:33Z

After discussion in our UI meeting today, I've gone ahead and removed the old code, which increased the code coverage, as that was most of what was not getting tested. I've tested in the emulator, and the dashboard is working well!

Abby-Wheelis · 2024-02-05T17:39:01Z

After resolving merge conflicts and one issue the merge created this cleanup PR is ready again!

shankari · 2024-02-08T07:00:21Z

Also, is there anywhere we still use ON_FOOT? I'm guessing this came from some previous iteration, as I don't think I've seen it in play as a base mode or label, is it still lurking somewhere? I suppose removing the code and allowing ON_FOOT to go to the default value is also an option.

The android raw values include ON_FOOT. I believe that the server code converts to WALKING when we look at inferred sections, but the raw and cleaned values can still have ON_FOOT. If you look at the real examples in the server tests, you will see many examples of the expected result (aka "ground truth") having ON_FOOT

Something else I've found while looking into the code coverage: the range - limits for modes. I believe this was something that might have influenced things like mode recommendations, but we don't currently have those, should I just take it out?

Yes, take it out. We used to use it to compute the "optimal" footprint, but that is not relevant any more and will likely have to be redesigned when we work with recommendations.

shankari

Almost there

www/__tests__/customMetricsHelper.test.ts

www/js/metrics/customMetricsHelper.ts

www/js/metrics/footprintHelper.ts

Abby Wheelis added 2 commits December 4, 2023 10:48

comments with calculations in the tests

0b471e5

from review, adding the calculations in comments with the tests helps to make the arithmetic more credible e-mission#1098 (review)

custom metrics - test numbers / keys

6947351

adding specific numbers from fakeLabels.json for the footprints, but keeping the testing at just the keys for mets due to complexity the keys in the fallback are different, so we can be sure here that the custom mets were accessed

cover edge cases in metHelper

23e0270

checking the 'ON_FOOT' case verifying negative speed returns 0, even if there is a fallback

Abby Wheelis added 2 commits December 4, 2023 15:33

increase coverage in footprintHelper

89fa035

added explicit throw of error and test to detect the thrown error, calculations now in try/catch block to account for errors also now allowing "ON FOOT" to go to default pending decision about "IN VEHICLE" for final coverage gap

no mets or footprint prior to initialization

75cb768

also renamed the clearing function to ensure it was clear it was only for testing

correct the failing test

f7fa822

test was failing because error was not thrown, it was handled, throwing error, which will be caught by the code that displays the metrics

Abby Wheelis added 3 commits December 5, 2023 14:15

removing range_limit

9cb6621

This was old code, and no longer used elected to take it out rather than test it, this concept is something we should keep in mind if we integrate suggestions back into the app (ie I can't e-bike from CO to GA, no matter how much emissions it would save)

add a "blank " label

3c68580

this label has no co2 or met information, which hits some of the edge cases in the custom metrics code

Abby-Wheelis marked this pull request as ready for review December 5, 2023 21:42

Abby-Wheelis and others added 2 commits February 5, 2024 10:17

Merge branch 'service_rewrite_2023' into metrics_cleanup

11e8636

update testing to resolve aftermath of merge conflict

315dabe

Abby-Wheelis changed the base branch from service_rewrite_2023 to master February 5, 2024 17:39

Abby-Wheelis changed the base branch from master to service_rewrite_2023 February 6, 2024 16:15

Abby-Wheelis changed the base branch from service_rewrite_2023 to master February 6, 2024 16:15

shankari requested changes Feb 8, 2024

View reviewed changes

www/__tests__/customMetricsHelper.test.ts Show resolved Hide resolved

www/js/metrics/customMetricsHelper.ts Show resolved Hide resolved

www/js/metrics/footprintHelper.ts Show resolved Hide resolved

Abby Wheelis added 4 commits February 9, 2024 16:22

adding quotes for consistency

eb33a98

prettier changes

46ab913

fixing formatting

2c8ffbb

restore catch for "IN_VEHICLE"

6bfef79

shankari approved these changes Feb 10, 2024

View reviewed changes

shankari merged commit 3d06350 into e-mission:master Feb 10, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🧹 📊 Metrics cleanup #1111

🧹 📊 Metrics cleanup #1111

Abby-Wheelis commented Dec 4, 2023

codecov bot commented Dec 4, 2023 •

edited

Loading

Abby-Wheelis commented Dec 4, 2023

Abby-Wheelis commented Dec 4, 2023

Abby-Wheelis commented Dec 4, 2023

Abby-Wheelis commented Dec 4, 2023

Abby-Wheelis commented Dec 5, 2023

Abby-Wheelis commented Feb 5, 2024

shankari commented Feb 8, 2024 •

edited

Loading

shankari left a comment

🧹 📊 Metrics cleanup #1111

🧹 📊 Metrics cleanup #1111

Conversation

Abby-Wheelis commented Dec 4, 2023

codecov bot commented Dec 4, 2023 • edited Loading

Codecov Report

Abby-Wheelis commented Dec 4, 2023

Abby-Wheelis commented Dec 4, 2023

Abby-Wheelis commented Dec 4, 2023

Abby-Wheelis commented Dec 4, 2023

Abby-Wheelis commented Dec 5, 2023

Abby-Wheelis commented Feb 5, 2024

shankari commented Feb 8, 2024 • edited Loading

shankari left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 4, 2023 •

edited

Loading

shankari commented Feb 8, 2024 •

edited

Loading