📊 🦶 Metrics Services Rewrites #1098

Abby-Wheelis · 2023-11-08T21:38:02Z

Just updated the list adding js/metrics-factory.js and js/metrics-mappings.js. Evidently, these 2 flew under the radar, as we did not rewrite them when we made the new dashboard -- so there's a bit more work left to do.
metrics-factory.js contains FootprintHelper and CalorieCal.
metrics.mappings.js contains CarbonDatasetHelper, METDatasetHelper, and CustomDatasetHelper.

To address these services, rewriting into 5 files:

carbonDatasets.ts
metDataset.ts
customMetricsHelper.ts
footprintHelper.ts
metHelper.ts

We need to make decisions about the country-divided carbon datasets and what to do about MET calculations see here. With these pending design decisions in mind, I plan to work with the footprint code cleanup and testing first.

angular -> ts migration for metrics-mappings and metrics-factory The datasets have their own files, there is a customhelper, a methelper, and a footprint helper Tested in the emulator and dashboard still behaves as expected

I was using the wrong function when I first converted

there is a similar function for the footprint, lets make sure they have different names

first pass of tests over the footprintHelper getFootprintForMetrics is one of the main functions, used to calculate displayed values for the dashboard screen

since we've changed the way fallback labels are handled, everything should be custom now. This means that we're not using the carbon values from the set country, but rather from the labels

after we take out the angular modules, we need to take out all the references to them as well

since we no longer rely on a set country for carbon calculations, but rather custom or default label configurations, we can take out the option to set it from the profile

we need to initialize the footprints before we can call them, accomplish this by calling the initialization function from the metrics tab once the app config is loaded

we no longer store user's height/weight/etc because we are not calculating calories burned, for that reason the code related to storing user's data or calculating calories is not needed we will keep MET handling so we can bin activity into high/med/low intensity later not currently using the "highestMET" calculation code, so commenting that out for now

Abby-Wheelis · 2023-11-09T17:23:38Z

I have now removed the calorie code and the fallback dataset code (keeping a singular fallback until I can test a few configs and be convinced that we ALWAYS use custom) according to the discussion in the issue.

I have also noticed a few more parts of the code that aren't used:

calculate highest MET (gets the highest MET in the list)
getLowestFootprintForDistance (finds lowest footprint for a given distance ignoring walk and bike)
getLowestMotorizedNonAirFootprint (similar to above, ignores air, walk and bike, and takes a threshold parameter for co2 by a range limited mode - ie not suggest you take an e-bike for your road trip?)
getOptimalDistanceRanges (seems to recommend optimal emissions for a trip by distance?)

These four methods are currently not used by our code, I see two options for me to proceed: (1) if we don't anticipate adding it back, take the code out or (2) keep the code and write tests for it if we anticipate wanting it in the app in a future update.

needed to include methods for clearing out the local variables, so added parameter to be able to set useCuston to false, and to clearHighest

Abby-Wheelis · 2023-11-09T22:22:41Z

I think I've sufficiently convinced myself that we'll always use the values in /www/json/label-options.json.sample, which will get coded as custom labels, since they're "walk, bike..." instead of "WALKING, BIKING.." Therefore we will never need the fallbacks, so I'm going to proceed with taking them out entirely.

This code should never be used at this point -- labels will default to those in the sample file, which means that they are "walk, bike, ect" -- custom labels rather than the default "WALKING, BICYCLING, etc" Since the labels are always some version of custom (even the fallback) those values will always be used over the country-specific fallback values

Abby-Wheelis · 2023-11-09T22:46:29Z

At this point, the files are rewritten and tested, ending with only 4 instead of the proposed 5 since we no longer are using the country-specific fallbacks.

The only thing that would remain to be done is testing the unused functions I mentioned above:

These four methods are currently not used by our code, I see two options for me to proceed: (1) if we don't anticipate adding it back, take the code out or (2) keep the code and write tests for it if we anticipate wanting it in the app in a future update.

Thoughts on removing vs keeping and testing? @JGreenlee @shankari

shankari · 2023-11-09T23:04:20Z

I don't think we should keep unused code around when it is not clear that we want to restore it.
If we need it again, we can get it from the history.

Abby-Wheelis · 2023-11-10T02:18:44Z

Not sure what happened, but the dashboard is no longer initializing properly (with the footprint dataset) and that screen is erroring out completely. I'll work on debugging more in the morning

…rics-services-rewrite

dataset was not initialized in time from metricsTab, lifting the call up to App fixed this issue

Abby-Wheelis · 2023-11-10T16:48:21Z

This wooorks now, but there's still lots of errors. I think it's ready for a preliminary review, as the errors seem to be the same ones I'm seeing on the services_rewrite_2023 branch. See video for the errors I'm seeing. There's the remote notifications one (expected for emulator), a few about populating trips/timeline, and then one about the metrics looking for ['$rootscope'] which I think is a known bug at this point - I don't think any of them are new from these changes, but feel free to point out otherwise!!

dashboardErrorsSmall.mov

JGreenlee · 2023-11-10T17:05:59Z

I think we should wait for #1086 and #1064 to be merged and then fix these.

Because, for example, I see an error about populateCompositeTrips - but #1086 is going to remove the need for that anyway.

…rics-services-rewrite

www/js/components/Chart.tsx

JGreenlee · 2023-11-17T19:18:52Z

www/js/metrics/footprintHelper.ts

+
+//variables for the highest footprint in the set and if using custom
+let highestFootprint = 0;
+let useCustom = false;


We decided this will always be true, right?
So we can remove this variable, along with setUseCustomFootprint, and simplify logic in a bunch of places

I remember thinking about doing this, but then was worried about what would happen if we hadn't initialized the custom footprint yet, you're right that we are always going to be custom at this point (either deployer-specified or from the sample custom files). I'm going to reevaluate and try and pull this out, but might end up needing some error handling.

This was less complex than I thought it would be. I elected to check that the custom footprint is defined before returning it in 'getFootprint`, that way we can still catch if the custom footprint is broken

Wondering now if we should apply the same treatment to the METs ... we still need the dataset, since it's what we pull from for met-equivalents but if the labels are always custom then the mets should also always be custom...

Concluded yes, the custom mets are populated when we initialized the custom dataset helper, and if they are not defined for some reason we can still fall back to the standard.

www/js/metrics/CustomMetricsHelper.ts

www/js/metrics/footprintHelper.ts

www/js/survey/multilabel/confirmHelper.ts

www/js/metrics/metHelper.ts

www/js/metrics/footprintHelper.ts

www/js/metrics/METDataset.ts

fixed typo, made single line, and added to horizontal chart as well e-mission#1098 (comment)

CustomMetricsHelper -> customMetricsHelper

removing the "is custom" variable for the footprint, and all references checking that footprint gotten is defined before returning, if not defined will throw an error

even though its an acronym, it is less confusing to have the name be lowercase since it is not a component

logDebug/logWarn instead of the console equivalents

logWarn in footprintHelper

JGreenlee · 2023-11-17T22:03:21Z

www/js/metrics/footprintHelper.ts

+          footprint['SUBWAY']) /
+          6) *


Ah, I had misread that
Maybe there's something we can do with the formatting to prevent confusion

we can store the sum and then divide?

I went ahead and pushed the change, with the way prettier chose to format the two, this is a line shorter! e626f6e

making sure the log statements here are thoughtful, renaming the input params to label options in the initialization code

The divisor being on it's own line was confusing, so now we are adding the vehicle modes and then dividing this should make the fact that it's an average more clear, and is one line shorter!

similar to the footprint, we will always use the custom METs, since the labels are custom (either deployer specified or our sample set) The custom mets are set up in the initialization of the custom dataset helper if the custom mets return undefined, will fall back to the standard mets

JGreenlee · 2023-11-19T04:38:27Z

www/__tests__/metHelper.test.ts

+  initCustomDatasetHelper(getConfig());
+  await new Promise((r) => setTimeout(r, 500));


What's with the setTimeout here? Wouldn't it be more 'proper' to do:

Suggested change

initCustomDatasetHelper(getConfig());

await new Promise((r) => setTimeout(r, 500));

const appConfig = await getConfig();

initCustomDatasetHelper(appConfig);

(Or is there some reason this wouldn't work?)

That would be cleaner/more proper to me, but I just tried it and it did not work - I think that's because appConfig is not the only async element of this workflow chain, it also calls getLabelOptions which is async.

getLabelOptions(newConfig).then((labelOptions)

what I can do though is set it up so that this function can be awaiting in the tests 51f6ece

This is much cleaner in testing, and still allows us to not hold up the app for it to load during initialization (because we don't care about the timing of the actual initialization)

JGreenlee · 2023-11-19T04:52:13Z

www/__mocks__/cordovaMocks.ts

+      if (key == 'config/app_ui_config') {
+        return new Promise<any>((rs, rj) =>
+          setTimeout(() => {
+            rs(fakeSurveyConfig);


Noting that some of this has since been changed in your other PR – I think it should all work out fine but just keep an eye on it

www/__tests__/customMetricsHelper.test.ts

in testing the initCustomDatasets function, I was calling it and then setting a timeout to wait for it to run, now I await getting the config, then await initialization double checked the Jest tests and running in the emulator - still going smoothly!

e-mission#1098 (comment)

There is no right or wrong here, but it's a bit cleaner and more consistent with the rest of the codebase if we generally follow this: i) for short one-liners, use const and declare an arrow function ii) for longer functions, use the traditional 'function' declaration also replaced some uses of 'var' with 'const' or 'let' added AppConfig type to initCustomDatasetHelper and replaced a console statement with logDebug

JGreenlee

LGTM!

codecov · 2023-12-03T20:27:03Z

Codecov Report

Merging #1098 (1006165) into service_rewrite_2023 (b900d1e) will increase coverage by 2.35%.
Report is 1 commits behind head on service_rewrite_2023.
The diff coverage is 76.06%.

Additional details and impacted files

@@                   Coverage Diff                    @@
##           service_rewrite_2023    #1098      +/-   ##
========================================================
+ Coverage                 56.44%   58.80%   +2.35%     
========================================================
  Files                        22       26       +4     
  Lines                      1311     1420     +109     
  Branches                    296      320      +24     
========================================================
+ Hits                        740      835      +95     
- Misses                      571      585      +14

Flag	Coverage Δ
unit	`58.80% <76.06%> (+2.35%)`	⬆️

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

Files	Coverage Δ
www/js/metrics/metDataset.ts	`100.00% <100.00%> (ø)`
www/js/survey/multilabel/confirmHelper.ts	`94.94% <ø> (+6.06%)`	⬆️
www/js/metrics/metHelper.ts	`81.81% <81.81%> (ø)`
www/js/metrics/footprintHelper.ts	`83.78% <83.78%> (ø)`
www/js/services/commHelper.ts	`23.72% <0.00%> (+0.19%)`	⬆️
www/js/metrics/customMetricsHelper.ts	`78.00% <78.00%> (ø)`

shankari

I didn't check the arithmetic in the tests. @Abby-Wheelis can you confirm that you checked them manually and they are correct? It would be helpful to add comments with the manual calculations for verification and in case the tests fail. I am not going to hold up the merge for this, but please deal with it next.
codecov has flagged new code that is not covered by tests, please handle them in a follow-on cleanup.

shankari · 2023-12-04T06:13:08Z

www/js/services/commHelper.ts

-    const fullUrl = `${window['$rootScope'].connectUrl}/${path}`;
-    data['aggregate'] = true;
+    const fullUrl = `${serverConnConfig.connectUrl}/${path}`;
+    query['aggregate'] = true;


Ah, so we don't store the connection config in the rootScope anymore since there is no rootScope.

shankari · 2023-12-04T06:29:25Z

www/js/metrics/footprintHelper.ts

+      result += footprint[mode] * mtokm(userMetrics[i].values);
+    } else if (mode == 'IN_VEHICLE') {
+      const sum =
+        footprint['CAR'] +


FYI: I first thought that we should be able to cache the value for IN_VEHICLE instead of recomputing it every time. But then I realized that this may actually want/need to be different for different time ranges given the upcoming change that takes the grid mix into account. Would be interesting to see what this looks like after that is done.

shankari · 2023-12-04T06:43:54Z

www/__tests__/customMetricsHelper.test.ts

+    walk: expect.any(Object),
+    bike: expect.any(Object),
+    bikeshare: expect.any(Object),


Is there a reason you are not checking against the actual numbers? These are going to be the same numbers as in fakeLabels.json, correct?

I probably did not do this originally, because I was mostly trying to test the keys. The default keys are WALKING, BICYCLING, etc, while the custom keys are walk, bike, etc.

I updated the footprint test, since that is a single value, but I have not updated the MET testing, since those objects are complex (some are speed dependent, etc), and taking the time to copy them over from fakeLabels.json seems like it would do more to test the test than the code, but I'll add a note about checking the keys!

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

Abby Wheelis added 12 commits November 8, 2023 14:24

convert metrics services

de49924

angular -> ts migration for metrics-mappings and metrics-factory The datasets have their own files, there is a customhelper, a methelper, and a footprint helper Tested in the emulator and dashboard still behaves as expected

display message instead of error

70f7238

I was using the wrong function when I first converted

reduce naming confusion

5108352

there is a similar function for the footprint, lets make sure they have different names

write footprinthelper tests

9118133

first pass of tests over the footprintHelper getFootprintForMetrics is one of the main functions, used to calculate displayed values for the dashboard screen

fix formatting

2cdd212

remove unused import

1a734f7

remove country-specific

1d7bf5b

since we've changed the way fallback labels are handled, everything should be custom now. This means that we're not using the carbon values from the set country, but rather from the labels

remove stale references

1eec588

after we take out the angular modules, we need to take out all the references to them as well

remove dataset setting

6102d62

since we no longer rely on a set country for carbon calculations, but rather custom or default label configurations, we can take out the option to set it from the profile

increase message readability

b075738

ensure initialization of footprints

ec4abff

we need to initialize the footprints before we can call them, accomplish this by calling the initialization function from the metrics tab once the app config is loaded

Abby Wheelis added 3 commits November 9, 2023 10:51

add tests for highest footprint

7ebcc94

needed to include methods for clearing out the local variables, so added parameter to be able to set useCuston to false, and to clearHighest

write tests for metHelper

3943053

custom dataset helper tests

7436502

Abby Wheelis added 3 commits November 9, 2023 16:47

remove unused code

8c28737

add docstrings

deb7104

resolve merge conflicts

9153a60

Abby Wheelis added 3 commits November 10, 2023 09:15

working to debug dashboard

9340297

Merge remote-tracking branch 'upstream/service_rewrite_2023' into met…

47daa3e

…rics-services-rewrite

lift dataset initialization

acc8022

dataset was not initialized in time from metricsTab, lifting the call up to App fixed this issue

Merge remote-tracking branch 'upstream/service_rewrite_2023' into met…

a587440

…rics-services-rewrite

JGreenlee requested changes Nov 17, 2023

View reviewed changes

Abby Wheelis and others added 11 commits November 17, 2023 13:56

clean up bugfix

1f495e2

fixed typo, made single line, and added to horizontal chart as well e-mission#1098 (comment)

renamed file

ef3bd59

CustomMetricsHelper -> customMetricsHelper

input Params -> labelOptions

df8ec72

footprint will always be custom

c160788

removing the "is custom" variable for the footprint, and all references checking that footprint gotten is defined before returning, if not defined will throw an error

rename the metDatasets file

4c1a4a6

even though its an acronym, it is less confusing to have the name be lowercase since it is not a component

name updates, just one dataset

b655f4a

prettier in Chart.tsx

dec322f

remove debugging log statements

a299574

logDebug statements

6272fb0

logDebug/logWarn instead of the console equivalents

logWarn instead of console.warn

6233af5

logWarn in footprintHelper

Rename CustomMetricsHelper.ts to customMetricsHelper.ts

5918b6b

JGreenlee reviewed Nov 17, 2023

View reviewed changes

Abby Wheelis added 3 commits November 17, 2023 15:06

cleaning up customMetrics

125faff

making sure the log statements here are thoughtful, renaming the input params to label options in the initialization code

clarify "in vehicle" calculations

e626f6e

The divisor being on it's own line was confusing, so now we are adding the vehicle modes and then dividing this should make the fact that it's an average more clear, and is one line shorter!

JGreenlee requested changes Nov 19, 2023

View reviewed changes

Abby Wheelis and others added 3 commits November 19, 2023 09:22

await instead of timeout

51f6ece

in testing the initCustomDatasets function, I was calling it and then setting a timeout to wait for it to run, now I await getting the config, then await initialization double checked the Jest tests and running in the emulator - still going smoothly!

Use expect.any(Type) for stricter testing

3dee26f

e-mission#1098 (comment)

JGreenlee approved these changes Nov 20, 2023

View reviewed changes

Merge branch 'service_rewrite_2023' into metrics-services-rewrite

bc2543b

Merge branch 'service_rewrite_2023' into metrics-services-rewrite

1006165

shankari approved these changes Dec 4, 2023

View reviewed changes

shankari merged commit c1f6dc3 into e-mission:service_rewrite_2023 Dec 4, 2023
4 checks passed

Abby-Wheelis pushed a commit to Abby-Wheelis/e-mission-phone that referenced this pull request Dec 4, 2023

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)

Abby-Wheelis mentioned this pull request Dec 4, 2023

🧹 📊 Metrics cleanup #1111

Merged

Abby-Wheelis mentioned this pull request Jan 3, 2024

🐛 Tracking errors on the new dashboard tab e-mission/e-mission-docs#986

Closed

shankari mentioned this pull request Feb 3, 2024

🎀 Final change of the service rewrite #1126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

📊 🦶 Metrics Services Rewrites #1098

📊 🦶 Metrics Services Rewrites #1098

Abby-Wheelis commented Nov 8, 2023

Abby-Wheelis commented Nov 9, 2023

Abby-Wheelis commented Nov 9, 2023

Abby-Wheelis commented Nov 9, 2023

shankari commented Nov 9, 2023

Abby-Wheelis commented Nov 10, 2023

Abby-Wheelis commented Nov 10, 2023

JGreenlee commented Nov 10, 2023

JGreenlee Nov 17, 2023

Abby-Wheelis Nov 17, 2023

Abby-Wheelis Nov 17, 2023

Abby-Wheelis Nov 17, 2023

Abby-Wheelis Nov 17, 2023

JGreenlee Nov 17, 2023

Abby-Wheelis Nov 17, 2023

Abby-Wheelis Nov 17, 2023

JGreenlee Nov 19, 2023

Abby-Wheelis Nov 19, 2023

JGreenlee Nov 19, 2023

JGreenlee left a comment

codecov bot commented Dec 3, 2023 •

edited

Loading

shankari left a comment

shankari Dec 4, 2023

shankari Dec 4, 2023

shankari Dec 4, 2023

Abby-Wheelis Dec 4, 2023

		initCustomDatasetHelper(getConfig());
		await new Promise((r) => setTimeout(r, 500));

📊 🦶 Metrics Services Rewrites #1098

📊 🦶 Metrics Services Rewrites #1098

Conversation

Abby-Wheelis commented Nov 8, 2023

Abby-Wheelis commented Nov 9, 2023

Abby-Wheelis commented Nov 9, 2023

Abby-Wheelis commented Nov 9, 2023

shankari commented Nov 9, 2023

Abby-Wheelis commented Nov 10, 2023

Abby-Wheelis commented Nov 10, 2023

JGreenlee commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JGreenlee left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 3, 2023 • edited Loading

Codecov Report

shankari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 3, 2023 •

edited

Loading