Cs/sc 3593 geo max distance and travel #34695

Charl1996 · 2024-05-30T09:09:35Z

Product Description

New criteria has been added to the geospatial disbursement algorithm section, namely:

Max distance from user
Max travel time
Travel mode (walk, cycle, drive)

The max travel time criteria is only available for the Road Network Algorithm as Mapbox gives us this information.

Technical Summary

It has been decided to incorporate the travel mode option as well so projects can more accurately apply the algorithm results in their own contexts. Also, adding the travel mode is low hanging fruit, since mapbox makes it easy to specify this.

Also, the max distance criteria uses kilometre since the metric system is not as widely used globally. Should we allow the user to specify the unit themselves?

Feature Flag

GEOSPATIAL and support_road_network_disbursement_algorithm

Safety Assurance

Safety story

Tested locally.

Automated test coverage

Unit tests included.

QA Plan

QA ticket

Migrations

The migrations in this code can be safely applied first independently of the code

Rollback instructions

This PR can be reverted after deploy with no further considerations

Labels & Review

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

zandre-eng

Looks good, just left a few comments.

corehq/apps/geospatial/forms.py

corehq/apps/geospatial/models.py

corehq/apps/geospatial/templates/map_visualization.html

zandre-eng

Thanks for the change, looks good from my end.

mkangia

Looks good, mostly non blocking feedback.

I believe using Kilometers only would be annoying to users.
Having something as default and then letting user change it would be fine.
Bonus would be to change the value based on what the user selects to they don't need to convert it themselves i.e if it was 25 KMs and user selects a different metric, update the 25 to the corresponding value for the new metric.

mkangia · 2024-05-31T04:51:34Z

corehq/apps/geospatial/forms.py

@@ -75,6 +79,24 @@ class Meta:
        required=False,
        min_value=1,
    )
+    max_case_distance = forms.IntegerField(
+        label=_("Max distance (km) to case"),
+        help_text=_("The maximum distance (in kilometer) from the user to the case. Leave blank to skip."),


nit: kilometers?

mkangia · 2024-05-31T04:52:22Z

corehq/apps/geospatial/migrations/0008_geoconfig_travel_mode.py

+            name='travel_mode',
+            field=models.CharField(choices=[('walking', 'Walking'), ('cycling', 'Cycling'), ('driving', 'Driving')], default='driving', max_length=50),
+        ),
+    ]


can we merge the two migrations into one?

I can do that

mkangia · 2024-05-31T04:52:57Z

corehq/apps/geospatial/models.py

@@ -68,6 +79,7 @@ class GeoConfig(models.Model):
    api_token = models.CharField(max_length=255, blank=True, null=True, db_column="api_token")

    def save(self, *args, **kwargs):
+        assert self.travel_mode in [self.WALKING, self.CYCLING, self.DRIVING]


isn't this already validated by the model since we add choices there?

Unfortunately not; one is still able to create a record with a different value.

okay. I believe the model supports adding validators as well in case you want to consider that but this is also okay

That's true. I forgot about those. Maybe I'll add it there

mkangia · 2024-05-31T04:55:09Z

corehq/apps/geospatial/routing_solvers/pulp.py

Just a general query, Changing calculations in the solver, how much does that effect HQ performance or the view performance?

@mkangia When you say "Changing calculations", do you refer to the user changing the travel mode for instance, or do you refer to us changing anything related to the calculations (adding additional constraints, etc)?

The user changing the travel mode for instance is something that's on mapbox in terms of performance. It doesn't affect HQ performance (although I haven't tested with massive amounts of data).

Adding additional constraint checking also doesn't seem to affect the performance in a meaningful way, but also, haven't tested with massive amounts of data.

I think the general expectation is that the disbursement runner might take a while (couple of seconds), hence why we have the spinner on the UI instead of hanging. I think it might be worth doing some sort of load test maybe and see what happens when we give it all the data we can (on the UI I think we limit it to 100 cases; not sure for CHWs).

I was just generally asking that the changes we make to the solver here, how much does that effect performance.
Could it become too slow leading to higher wait time for user on HQ UI or the page to itself could take too long to load.

not a blocker here for this PR.

Oh, I see. No, the changes does not affect load time at all. The only affect it might have is when actually running the disbursement.

right, and that could take a while. You say we limit to 100 cases, so we only calculate for 100 cases at once in one request?

so we only calculate for 100 cases at once in one request?

Correct

Charl1996 · 2024-05-31T05:59:57Z

@mkangia

Having something as default and then letting user change it would be fine.

So are you suggesting that we allow the user to set up their preference w.r.t. metric or imperial?

mkangia · 2024-05-31T06:10:36Z

So are you suggesting that we allow the user to set up their preference w.r.t. metric or imperial?

Does mapbox support all metrics or just kms and then we would need to convert it for the user?
I do think some users would be habitual of using say, miles, and would get annoyed if they have to convert it to kms every time they want to set this value.

Charl1996 · 2024-05-31T06:17:31Z

Does mapbox support all metrics

A brief look at their docs it doesn't seem like they support other metrics.

mkangia · 2024-05-31T06:21:35Z

A brief look at their docs it doesn't seem like they support other metrics.

Cool. Fine to leave it for feedback from others then.

Charl1996 · 2024-06-03T10:06:21Z

Just force pushed due to a rebase where I added all the migrations into one file.

mkangia · 2024-06-03T16:29:17Z

corehq/apps/geospatial/forms.py

@@ -96,6 +96,7 @@ class Meta:
        help_text=_("The travel mode of the users. "
                    "Consider this when specifying the max travel time to each case."),
        widget=Select(choices=GeoConfig.VALID_TRAVEL_MODES),
+        validators=[validate_travel_mode]


oh actually, what is this? In forms I believe the recommended way is to use clean_ methods.
On models, we have validators.

@mkangia I followed this example from the django docs. But maybe if you think I should rather move it to the clean methods I can do that?

I see in HQ there are a couple of instances where we use this paradigm (here and here).

Cool. I might be limited by what I know here then. But this is better than calling assert.

Cs/sc 3385 remove redundant code

…rsement-run Show algorithm parameters when running disbursement

Charl1996 · 2024-06-05T09:46:20Z

Merged #34696 and #34716 into this branch

mkangia

Reviewed last three commits that seem to be new after the two merges.

Charl1996 added awaiting QA QA in progress. Do not merge product/feature-flag Change will only affect users who have a specific feature flag enabled labels May 30, 2024

Charl1996 requested review from kaapstorm, mkangia, zandre-eng and ajeety4 May 30, 2024 09:09

dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label May 30, 2024

Charl1996 marked this pull request as ready for review May 30, 2024 09:17

zandre-eng reviewed May 30, 2024

View reviewed changes

corehq/apps/geospatial/forms.py Show resolved Hide resolved

corehq/apps/geospatial/models.py Outdated Show resolved Hide resolved

corehq/apps/geospatial/templates/map_visualization.html Show resolved Hide resolved

Charl1996 mentioned this pull request May 30, 2024

Cs/sc 3385 remove redundant code #34696

Merged

3 tasks

zandre-eng approved these changes May 30, 2024

View reviewed changes

mkangia reviewed May 31, 2024

View reviewed changes

Charl1996 added 7 commits June 3, 2024 11:45

Add new criteria to geo settings

a25b118

Support travel modes

2626f0a

Handle unassigned disbursement results more explicitly

9fd2ae2

Fix up tests

ac15106

Add tests for new criteria

2b5ee29

Fix test

12dc0d1

Check travel_mode before saving

d501221

Charl1996 force-pushed the cs/SC-3593-geo-max-distance-and-travel branch from 0df6b8c to d501221 Compare June 3, 2024 10:05

Add travel_mode validation to form

13e6ace

zandre-eng approved these changes Jun 3, 2024

View reviewed changes

mkangia reviewed Jun 3, 2024

View reviewed changes

Charl1996 added 2 commits June 4, 2024 11:01

Strip out redundant code

ebc5316

Update tests

b27690e

Remove polling code from javascript

07f6c02

mkangia approved these changes Jun 5, 2024

View reviewed changes

Charl1996 added 7 commits June 5, 2024 09:29

Merge pull request #34696 from dimagi/cs/SC-3385-remove-redundant-code

382c80d

Cs/sc 3385 remove redundant code

Return and show parameters on disbursement run

49db7c0

Remove 'remove me' code

f79d2d8

Fix linting

b2f914f

Remove calculated method from dataclass

d82460b

Refactor slightly

e261b88

Merge pull request #34716 from dimagi/cs/SC-3612-show-params-on-disbu…

92f789e

…rsement-run Show algorithm parameters when running disbursement

Fix lint

a15d883

Charl1996 added QA Passed and removed awaiting QA QA in progress. Do not merge labels Jun 18, 2024

Charl1996 added 2 commits June 18, 2024 11:36

Merge branch 'master' into cs/SC-3593-geo-max-distance-and-travel

f7b34cf

Fix flake8 linting issues

503aaa0

Charl1996 requested review from mkangia and zandre-eng June 18, 2024 11:02

mkangia approved these changes Jun 18, 2024

View reviewed changes

Charl1996 merged commit 600c11b into master Jun 19, 2024
13 checks passed

mkangia deleted the cs/SC-3593-geo-max-distance-and-travel branch July 16, 2024 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cs/sc 3593 geo max distance and travel #34695

Cs/sc 3593 geo max distance and travel #34695

Charl1996 commented May 30, 2024 •

edited

Loading

zandre-eng left a comment

zandre-eng left a comment

mkangia left a comment

mkangia May 31, 2024

mkangia May 31, 2024

Charl1996 May 31, 2024

mkangia May 31, 2024

Charl1996 May 31, 2024

mkangia May 31, 2024

Charl1996 May 31, 2024

mkangia May 31, 2024

Charl1996 May 31, 2024

Charl1996 May 31, 2024

mkangia May 31, 2024

Charl1996 May 31, 2024

mkangia May 31, 2024

Charl1996 May 31, 2024

Charl1996 commented May 31, 2024

mkangia commented May 31, 2024

Charl1996 commented May 31, 2024

mkangia commented May 31, 2024

Charl1996 commented Jun 3, 2024

mkangia Jun 3, 2024

mkangia Jun 3, 2024

Charl1996 Jun 4, 2024

Charl1996 Jun 4, 2024 •

edited

Loading

mkangia Jun 4, 2024

Charl1996 commented Jun 5, 2024

mkangia left a comment

Cs/sc 3593 geo max distance and travel #34695

Cs/sc 3593 geo max distance and travel #34695

Conversation

Charl1996 commented May 30, 2024 • edited Loading

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Migrations

Rollback instructions

Labels & Review

zandre-eng left a comment

Choose a reason for hiding this comment

zandre-eng left a comment

Choose a reason for hiding this comment

mkangia 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

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

Charl1996 commented May 31, 2024

mkangia commented May 31, 2024

Charl1996 commented May 31, 2024

mkangia commented May 31, 2024

Charl1996 commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charl1996 Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charl1996 commented Jun 5, 2024

mkangia left a comment

Choose a reason for hiding this comment

Charl1996 commented May 30, 2024 •

edited

Loading

Charl1996 Jun 4, 2024 •

edited

Loading