Update carbon calculations for GIS server (richer modes) #576

tms-martins · 2019-06-03T09:42:05Z

Update the carbon calculations so that they use the richer modes in the GIS-based server (ground_truth_matching branch).

Aggregate (average) data won't work properly on local servers, due to an outstanding issue.
In fillFootprintAggVals it can happen that that the mode ON_FOOT has a NaN value; a workaround was provided for that, but the source of the problem should be investigated (perhaps it is due to different modes in app VS server?)

…ev-app

AmplabJenkins · 2019-06-03T09:43:50Z

Can one of the admins verify this patch?

shankari · 2019-06-03T16:48:35Z

Aggregate (average) data won't work properly on local servers, due to an outstanding issue.

e-mission/e-mission-docs#288 is resolved (you need to modify the CSP in index.html). I had left it open to track potential enhancements, but now I've opened e-mission/e-mission-docs#408 instead.

shankari · 2019-06-03T16:54:44Z

In fillFootprintAggVals it can happen that that the mode ON_FOOT has a NaN value; a workaround was provided for that, but the source of the problem should be investigated (perhaps it is due to different modes in app VS server?)

Yes, the server supports both ON_FOOT and WALKING for sensed modes (the ones read from the phone API and smoothed) (emission/core/wrapper/motionactivity.py).

When the mode inference step is run successfully, both ON_FOOT and WALKING are collapsed to WALKING (emission/core/wrapper/modeprediction.py). You should assume that the mode is almost always WALKING with a fallback to ON_FOOT if it turns out that the mode inference step has been turned off or is not working for some reason.

shankari

First round of obvious changes. Will do a more careful review of the real functionality changes in a bit.

shankari · 2019-06-03T17:26:25Z

www/index.html

@@ -106,6 +106,8 @@
    <script src="lib/angular-nvd3/dist/angular-nvd3.min.js"></script>
    <script src="lib/nz-tour/dist/nz-tour.min.js"></script>
    <link rel="stylesheet" href="lib/nvd3/build/nv.d3.css">
+    <!-- TIAGO: allow debug with weinre -->
+    <script src="http://192.168.1.238:8090/target/target-script-min.js#anonymous"></script>


Remove? Seems to be hardcoded to your IP.

shankari · 2019-06-03T17:31:55Z

www/js/metrics-factory.js

+    return result;
+  }
+  fh.getLowestFootprintForDistance = function(distance) {
+    var lowestFootprint = 9999;


Why 9999 instead of Number.MAX_VALUE?

www/js/metrics-factory.js

shankari · 2019-06-03T17:34:55Z

www/js/metrics.js

@@ -410,6 +410,9 @@ angular.module('emission.main.metrics',['nvd3', 'emission.services', 'ionic-date
      delete clonedData.metric;
      clonedData.metric_list = [DURATION, MEDIAN_SPEED, COUNT, DISTANCE];
      clonedData.is_return_aggregate = true;
+      // TIAGO we should use our own server here, but for now it will return empty metrics, see:


I responded to this.

shankari · 2019-06-03T17:35:29Z

www/js/metrics.js

+        lastWeekCarbonInt               = FootprintHelper.getFootprintForMetrics(userCarbonData);
+      }
+      else {
+        // TIAGO: -------------


nit: remove?

I will remove these and similar before the final PR (they have my name on them so I can find them quickly).

www/json/connectionConfig.json

www/templates/metrics/metrics-control.html

shankari · 2019-06-03T17:40:02Z

Yes, the server supports both ON_FOOT and WALKING for sensed modes (the ones read from the phone API and smoothed) (emission/core/wrapper/motionactivity.py).

So I realized that we would need to handle both ON_FOOT and WALKING correctly anyway for the calorie calculations, so I went looking for it, and I found https://github.com/e-mission/e-mission-phone/blob/master/www/js/metrics.js#L832

So it looks like we convert all WALKING to ON_FOOT anyway. Given that, I am not sure why this should be NaN. Since you are still connecting to https://e-mission.eecs.berkeley.edu for the aggregates, I can try to debug this if you file a new issue.

shankari

Just a few more fit and finish issues but rest looks good.
Functionality is fine, and if you file an issue for the NaN with more details on how to reproduce, I am happy to take a look.

www/js/metrics-factory.js

www/js/metrics.js

www/js/metrics-factory.js

shankari · 2019-06-10T15:43:29Z

www/js/metrics.js

          }
        }
+
+        $scope.carbonData.aggrVehicleRange = FootprintHelper.getFootprintForMetrics(aggrCarbonData);


Not your change, but I don't see this variable being used anywhere.
So you could remove it, since aggrCarbon seems to represent the same value anyway.

If not should probably rename to indicate that it is not a range any more.

tms-martins · 2019-06-11T09:16:31Z

When the mode inference step is run successfully, both ON_FOOT and WALKING are collapsed to WALKING (emission/core/wrapper/modeprediction.py). You should assume that the mode is almost always WALKING with a fallback to ON_FOOT if it turns out that the mode inference step has been turned off or is not working for some reason.

Does this mean that in the app I will always use WALKING, including in the var footprint in metrics-factory.js; and that any label ON_FOOT should be treated as WALKING or even converted (in app) to 'WALKING'?

…e IN_VEHICLE restored

www/js/metrics-factory.js

tms-martins · 2019-06-17T10:38:50Z

When the mode inference step is run successfully, both ON_FOOT and WALKING are collapsed to WALKING (emission/core/wrapper/modeprediction.py). You should assume that the mode is almost always WALKING with a fallback to ON_FOOT if it turns out that the mode inference step has been turned off or is not working for some reason.

Does this mean that in the app I will always use WALKING, including in the var footprint in metrics-factory.js; and that any label ON_FOOT should be treated as WALKING or even converted (in app) to 'WALKING'?

@shankari need your input on this, I think it's the only outstanding issue in the PR - besides removing all my test code (e.g. console.debug()'s) of course 😉

www/js/metrics-factory.js

shankari · 2019-06-17T16:50:13Z

www/index.html

@@ -3,7 +3,7 @@
  <head>
    <meta charset="utf-8">
    <meta name="viewport" content="initial-scale=1, maximum-scale=1, user-scalable=no, width=device-width">
-    <meta http-equiv="Content-Security-Policy" content="default-src 'self' data: gap: https://ssl.gstatic.com http://nominatim.openstreetmap.org https://e-mission.eecs.berkeley.edu https://api.ionic.io/push/tokens emission: 'unsafe-eval'; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; img-src 'self' data: http://*.tile.openstreetmap.org http://tile.stamen.com https://*.tile.stamen.com http://*.tile.stamen.com 'unsafe-inline'">
+    <meta http-equiv="Content-Security-Policy" content="default-src 'self' data: gap: https://ssl.gstatic.com http://nominatim.openstreetmap.org http://192.168.1.238 https://api.ionic.io/push/tokens emission: 'unsafe-eval'; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; img-src 'self' data: http://*.tile.openstreetmap.org http://tile.stamen.com https://*.tile.stamen.com http://*.tile.stamen.com 'unsafe-inline'">


also remove from PR :)

shankari · 2019-06-17T16:54:51Z

@tms-martins
Sorry for the late review; had to turn in zeroth draft of thesis last night :)

Does this mean that in the app I will always use WALKING, including in the var footprint in metrics-factory.js; and that any label ON_FOOT should be treated as WALKING or even converted (in app) to 'WALKING'?

We already do this conversion in the app, but in the reverse direction.
https://github.com/e-mission/e-mission-phone/blob/master/www/js/metrics.js#L832

Sorry about the lack of consistency :(

I can fix that or you can fix it as part of the PR.
At the end, we should use WALKING everywhere, and the app should convert any ON_FOOT to WALKING

LMK what you prefer.

Making the change myself since it is a comment-only change

i18n e-mission-phone

tms-martins · 2019-06-24T08:59:12Z

We already do this conversion in the app, but in the reverse direction.
https://github.com/e-mission/e-mission-phone/blob/master/www/js/metrics.js#L832

At the end, we should use WALKING everywhere, and the app should convert any ON_FOOT to WALKING

LMK what you prefer.

When using the GIS-based server, it seems that ON_FOOT is only introduced there in getDataFromMetrics() since all the data I get from the server uses WALKING. So I would simply remove this conversion, if it doesn't break anything else (?).

It also begs the question: should I ever expect the mode RUNNING? If so, I have to either provide a carbon metric for this, or treat it as WALKING when calculating carbon footprints.

Nevertheless, should I always check server data for old modes (i.e. ON_FOOT) and replace with new ones (i.e. WALKING) before proceeding?

…ev-app

…e IN_VEHICLE restored

…NG conversions

…artins/e-mission-phone into update_carbon_calc_for_GIS

tms-martins · 2019-06-25T09:31:05Z

www/js/metrics.js

-          if (aggrCarbonData[i].key === "IN_VEHICLE") {
-            $scope.carbonData.aggrVehicleRange = FootprintHelper.getFootprintRaw(aggrCarbonData[i].values, aggrCarbonData[i].key);
-            $scope.carbonData.aggrCarbon = FootprintHelper.getFootprint(aggrCarbonData[i].values, aggrCarbonData[i].key);
+          if (isNaN(aggrCarbonData[i].values)) {


This has been investigated a bit more using the master phone branch and turned into an issue.
e-mission/e-mission-docs#422

shankari

Although both master and your branch now have the changes in #584, the github file changes list still shows the changes in that PR mixed with your changes.

So I'm using a new compare
master...tms-martins:update_carbon_calc_for_GIS for this review.

Looks good barring the hardcoded IP addresses and TIAGO messages. Happy to merge once you have resolved those. Can you also add some testing screenshots or a testing video?

I am now investigating e-mission/e-mission-docs#422

www/json/connectionConfig.json

shankari · 2019-06-29T01:03:36Z

www/js/metrics.js

-                      fieldPhone = "ON_FOOT";
-                    } else {
+                    // if (field === "WALKING" || field === "RUNNING") {
+                    //   fieldPhone = "ON_FOOT";


if you are using WALKING everywhere, you should probably have the reverse of this, right? e.g. if returned as ON_FOOT, convert to WALKING.

Or I guess you are handing this in the footprint calculations (e.g. getFootprintForMetrics). Any reason for the change?

I knew I didn't want to convert WALKINg to ON_FOOT; but removing/commenting that code was also lazy, because then I can have RUNNING labels which aren't handled in getFootprintForMetrics. I think a separate branch should address the mode consistency (i.e. check for and convert certain modes) at some point.
I will restore this line and avoid addressing the issue piecemeal before I break something :)

tms-martins · 2019-07-01T09:11:50Z

I'm tidying up now, including removing the connectionConfig.json.
It may come as a surprise at this point, but I'm not using the complete toolchain. I've been developing and testing using the DevApp on an Android device, and using Weinre to view the console and inspect UI elements. If you feel that this may open up the possibility of inconsistent behavior once the app is actually built for Android/iOS please let me know.

tms-martins · 2019-07-01T09:22:32Z

the github file changes list still shows the changes in that PR mixed with your changes.

Yes. I rebased my branch, and when I looked at the GitHub diff last time all looked well. I don't know exactly why this happens, although I've seen it before. I'm afraid that if I try to fix it now I end up making matters worse.

tms-martins · 2019-07-01T09:52:41Z

Tidied up. @shankari what is the best way to add testing videos? Something like G Drive or is there a Git Repo for those?

tms-martins · 2019-07-01T10:02:04Z

the github file changes list still shows the changes in that PR mixed with your changes.

Yes. I rebased my branch, and when I looked at the GitHub diff last time all looked well. I don't know exactly why this happens, although I've seen it before. I'm afraid that if I try to fix it now I end up making matters worse.

I've read up that the PR-diff is made "against the common ancestor of the source branch and the target branch". Also that a quick fix is to use the edit button to temporarily change the base to something other than master and then back again, which forces Git to update the PR.

shankari · 2019-07-01T14:25:19Z

I'm afraid that if I try to fix it now I end up making matters worse.

No worries. I can just review off the compare since I don't expect to have a lot of review comments.

shankari · 2019-07-01T14:26:23Z

Tidied up. @shankari what is the best way to add testing videos? Something like G Drive or is there a Git Repo for those?

G Drive is fine. I typically make the video short and gzip it so that I can upload it to github directly (.gz extensions are supported).

shankari · 2019-07-01T14:27:40Z

I've been developing and testing using the DevApp on an Android device, and using Weinre to view the console and inspect UI elements. If you feel that this may open up the possibility of inconsistent behavior once the app is actually built for Android/iOS please let me know.

This should not be an issue. I also use the devapp for my development and it has been pretty reliable in translating to real devices.

shankari

Otherwise all looks good. Will merge as soon as I see testing video.

www/index.html

www/js/metrics.js

…artins/e-mission-phone into update_carbon_calc_for_GIS

www/index.html

shankari

I don't see anything else obviously wrong. Will merge as soon as I see testing video :)

tms-martins · 2019-07-02T09:52:08Z

@shankari Testing video is here. I'm not sure if there were specific guidelines to follow; in case I'm missing something please let me know.

shankari · 2019-07-02T13:33:44Z

Video looks good.

tms-martins added 3 commits May 13, 2019 15:24

added connection config file with 0.0.0.0 instead of localhost, for d…

0ed9378

…ev-app

updated trip modes and carbon data, refactored main carbon calculations

f929f1b

restored optimal and worst carbon footprint calculations

3f79598

tms-martins mentioned this pull request Jun 3, 2019

Carbon calculations on phone e-mission/e-mission-docs#396

Closed

shankari requested changes Jun 3, 2019

View reviewed changes

shankari requested changes Jun 10, 2019

View reviewed changes

a few corrections after first round of PR review, carbon calc for mod…

315703b

…e IN_VEHICLE restored

tms-martins commented Jun 11, 2019

View reviewed changes

www/js/metrics-factory.js Outdated Show resolved Hide resolved

restored IN_VEHICLE to standardMETs

68d66af

DO3B added 3 commits June 17, 2019 04:57

Add i18n for most of the app

d8e0a27

Fix issues and add consent files management

a552c10

Fix e-mission#4

ff15d04

shankari requested changes Jun 17, 2019

View reviewed changes

DO3B and others added 7 commits June 18, 2019 00:23

updatecheck i18n and minor fixes

83d4a56

Add forgotten key

29bfaf7

Add i18n for modes and purposes

91d8d8b

Change loadAndPopulateOptions to use lang

5b94703

Change static formats by locale aware formats

f8db039

Change comment to match reality

07982ac

Making the change myself since it is a comment-only change

Merge pull request e-mission#584 from fabmob/i18n-emission

f03c1ff

i18n e-mission-phone

tms-martins added 2 commits June 24, 2019 11:20

added connection config file with 0.0.0.0 instead of localhost, for d…

01c8f6c

…ev-app

updated trip modes and carbon data, refactored main carbon calculations

f0df6a0

tms-martins added 6 commits June 24, 2019 11:27

restored optimal and worst carbon footprint calculations

546f083

a few corrections after first round of PR review, carbon calc for mod…

d9cd5e3

…e IN_VEHICLE restored

restored IN_VEHICLE to standardMETs

7b5aad0

Always use WALKING instead of ON_FOOT; other small celan-ups

e3746e1

updated base (rebased on master after PR 584), added ON_FOOT to WALKI…

dc929da

…NG conversions

Merge branch 'update_carbon_calc_for_GIS' of https://github.com/tms-m…

3cc0597

…artins/e-mission-phone into update_carbon_calc_for_GIS

tms-martins commented Jun 25, 2019

View reviewed changes

shankari requested changes Jun 29, 2019

View reviewed changes

Tidied up for PR/merge.

a4737e2

shankari requested changes Jul 1, 2019

View reviewed changes

www/index.html Show resolved Hide resolved

www/js/metrics.js Outdated Show resolved Hide resolved

shankari and others added 3 commits July 1, 2019 07:45

Restore the e-mission server to the CSP

30e661d

Fixed property names (missing quotes)

8220ddb

Merge branch 'update_carbon_calc_for_GIS' of https://github.com/tms-m…

fe2c575

…artins/e-mission-phone into update_carbon_calc_for_GIS

tms-martins commented Jul 1, 2019

View reviewed changes

www/index.html Show resolved Hide resolved

Corrected/restored commented code

c34dd15

shankari approved these changes Jul 1, 2019

View reviewed changes

shankari mentioned this pull request Jul 2, 2019

Unify the implementations of getDataFromMetrics #591

Merged

shankari merged commit e0b6169 into e-mission:master Jul 2, 2019

Update carbon calculations for GIS server (richer modes) #576

Update carbon calculations for GIS server (richer modes) #576

Conversation

tms-martins commented Jun 3, 2019

AmplabJenkins commented Jun 3, 2019

shankari commented Jun 3, 2019

shankari commented Jun 3, 2019

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

Choose a reason for hiding this comment

shankari commented Jun 3, 2019

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

tms-martins commented Jun 11, 2019 • edited Loading

tms-martins commented Jun 17, 2019

Choose a reason for hiding this comment

shankari commented Jun 17, 2019 • edited Loading

tms-martins commented Jun 24, 2019

Choose a reason for hiding this comment

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

tms-martins commented Jul 1, 2019

tms-martins commented Jul 1, 2019

tms-martins commented Jul 1, 2019

tms-martins commented Jul 1, 2019

shankari commented Jul 1, 2019

shankari commented Jul 1, 2019

shankari commented Jul 1, 2019 • edited Loading

shankari left a comment

Choose a reason for hiding this comment

shankari left a comment

Choose a reason for hiding this comment

tms-martins commented Jul 2, 2019

shankari commented Jul 2, 2019

tms-martins commented Jun 11, 2019 •

edited

Loading

shankari commented Jun 17, 2019 •

edited

Loading

shankari commented Jul 1, 2019 •

edited

Loading