Add group name to WeightModifier. Add ability to use custom factor ag… #963

nadiia-volyk · 2023-04-12T09:15:55Z

…gregator function for weight modifiers groups

BACKGROUND:

Sometimes need to find max(not min as usual) factor for edge with several WeightModifiers. Need polyline weight modifier

DESCRIPTION:

Add group name to WeightModifier
Add ability to use custom factor aggregator function for weight modifiers groups. If an edge meets the condition of several WeightModifier objects they will be grouped by group name and factor aggregator function will be applied to WeightModifiers factors. By default - the lowest factor of group is chosen. Finally, factors of all groups will be multiplied.
Add SetWeightModifiersGroupFactorAggregator method for setting factor aggregator functions for different groups. AggregateFactorMin, AggregateFactorMax, AggregateFactorMultiply can be used.
Add method for creating polyline weight modifier

REQUIRED:

All changes are up to date in CHANGELOG.md.

This change is

…gregator function for weight modifiers groups

DmytroMuravskyi

Don't forget about CHANGELOG.md.

Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @nadiia-volyk)

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 14 at r1 (raw file):

            new Dictionary<string, WeightModifier>();

        private Dictionary<string, Func<double, double, double>> _groupFactorAggregators =

I'm not sure if we need another dictionary. Maybe modifiers just can share a group object?

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 125 at r1 (raw file):

                name,
                new Func<Vertex, Vertex, bool>((a, b) =>
                {

This function is way to expensive for something that is called for every edge, not every line is dependent on input parameters a and b.
Try to move as much as possible outside of lambda and just pass precomputed data as capture list. As an alternative, new type can be inherited from WeightModifier

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 141 at r1 (raw file):

                    var hintLine = new RoutingHintLine(polyline, factor, influenceDistance, true, is2D);
                    var isAffectedParallely = hintLine.Affects(a.Point, b.Point);
                    var isIntersected = polyline.Intersects(line, out _, includeEnds: true);

Check if Line DistanceTo Line is more efficient in this case, as intersect won't work with influence distance.

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 163 at r1 (raw file):

        {
            var modifiersGroups = _weightModifiers.Values.GroupBy(m => m.Group);
            var modifierFactors = new List<double>();

The function is called for every edge. There is no reason to do grouping every time. This can be function that operates on one group of modifiers instead.

Code quote:

ModifierFactor

DmytroMuravskyi · 2023-04-13T09:04:08Z

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs

+        /// <param name="groupName">Group name. Must be not null.</param>
+        /// <returns>List of WeightModifier with the specified group name.</returns>
+        /// <exception cref="ArgumentNullException">Throws if <paramref name="groupName"/> is null.</exception>
+        public List<WeightModifier> GetGroupWeightModifiers(string groupName)


Rename to GetWeightModifiersGroup to be consistent with function above and return IEnumerable and ToList is not required.

DmytroMuravskyi

Reviewable status: 0 of 1 approvals obtained (waiting on @nadiia-volyk)

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 66 at r1 (raw file):

        /// <param name="groupName">Group name.</param>
        /// <param name="groupFactorAggregator">Factor aggregator function.</param>
        public void SetWeightModifiersGroupFactorAggregator(string groupName, Func<double, double, double> groupFactorAggregator)

I think we can drop Factor from name to make it little bit shorter.

nadiia-volyk

Reviewable status: 0 of 1 approvals obtained (waiting on @DmytroMuravskyi)

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 66 at r1 (raw file):

Previously, DmytroMuravskyi wrote…

I think we can drop Factor from name to make it little bit shorter.

Done.

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 77 at r1 (raw file):

Previously, DmytroMuravskyi wrote…

Rename to GetWeightModifiersGroup to be consistent with function above and return IEnumerable and ToList is not required.

Done.

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 125 at r1 (raw file):

Previously, DmytroMuravskyi wrote…

This function is way to expensive for something that is called for every edge, not every line is dependent on input parameters a and b.
Try to move as much as possible outside of lambda and just pass precomputed data as capture list. As an alternative, new type can be inherited from WeightModifier

Done.

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 141 at r1 (raw file):

Previously, DmytroMuravskyi wrote…

Check if Line DistanceTo Line is more efficient in this case, as intersect won't work with influence distance.

Done.

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 163 at r1 (raw file):

Previously, DmytroMuravskyi wrote…

The function is called for every edge. There is no reason to do grouping every time. This can be function that operates on one group of modifiers instead.

Done.

wynged · 2023-04-13T16:35:19Z

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs

@@ -51,14 +55,43 @@ public void ClearWeightModifiers()
            _weightModifiers.Clear();
        }

+        /// <summary>
+        /// Set factor aggregator function for weight modifiers group.


let's describe a little bit more about what these do. "The aggregator function will be applied to any list of weight modifications that share a group name. This ensures that if a type of modifier is applied to a single edge more than once we have options for how to apply duplicate modifiers."

wynged · 2023-04-13T16:40:14Z

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs

+            }
+            catch
+            {
+                hintLine = null;


what can result in us not being able to make a hint line? should we notify the user somehow that this happened?

wynged · 2023-04-13T16:41:55Z

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs

+        /// Group weight modifiers.
+        /// </summary>
+        /// <returns>Weight modifiers grouped by group name.</returns>
+        private IEnumerable<IGrouping<string, WeightModifier>> GroupWeightModifiers()


rename to GroupedWeightModifiers

DmytroMuravskyi

Reviewed 3 of 4 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nadiia-volyk)

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 125 at r1 (raw file):

Previously, nadiia-volyk wrote…

Done.

Few details:

Do not null check we don't really expect that it's better to throw so the problem is visible. Also, just return null above if hint is null
Run Affects first since it doesn't require any additional data.

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 217 at r2 (raw file):

            }

            if (!_groupFactorAggregators.TryGetValue(modifiersGroup.Key, out var aggregateFactorFunc))

Looks good, let's also make this function have two parameters instead of group: IEnumerable and AggregateFunction.
And retrieve AggregateFunction in CalculateEdgeInfos.

DmytroMuravskyi

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nadiia-volyk)

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 125 at r1 (raw file):

Previously, DmytroMuravskyi wrote…

Few details:

Do not null check we don't really expect that it's better to throw so the problem is visible. Also, just return null above if hint is null

Run Affects first since it doesn't require any additional data.

Also, consider grouping code into something like hintLine.Intersects(a ,b)

nadiia-volyk

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @DmytroMuravskyi and @wynged)

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 125 at r1 (raw file):

Previously, DmytroMuravskyi wrote…

Also, consider grouping code into something like hintLine.Intersects(a ,b)

Done.

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 59 at r2 (raw file):

Previously, wynged (Eric Wassail) wrote…

let's describe a little bit more about what these do. "The aggregator function will be applied to any list of weight modifications that share a group name. This ensures that if a type of modifier is applied to a single edge more than once we have options for how to apply duplicate modifiers."

Done.

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 129 at r2 (raw file):

Previously, wynged (Eric Wassail) wrote…

what can result in us not being able to make a hint line? should we notify the user somehow that this happened?

if the polyline is vertical and needs to be used in 2D comparison, such hint line cannot be created.

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 172 at r2 (raw file):

Previously, wynged (Eric Wassail) wrote…

rename to GroupedWeightModifiers

Done.

Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRoutingWeightModifier.cs line 217 at r2 (raw file):

Previously, DmytroMuravskyi wrote…

Looks good, let's also make this function have two parameters instead of group: IEnumerable and AggregateFunction.
And retrieve AggregateFunction in CalculateEdgeInfos.

Done.

wynged

Reviewed 2 of 3 files at r3, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @DmytroMuravskyi and @nadiia-volyk)

Add group name to WeightModifier. Add ability to use custom factor ag…

e041b45

…gregator function for weight modifiers groups

DmytroMuravskyi reviewed Apr 12, 2023

View reviewed changes

DmytroMuravskyi reviewed Apr 13, 2023

View reviewed changes

nadiia-volyk added 2 commits April 13, 2023 16:30

Refactoring

3f5635f

Update changelog

3f0abaa

nadiia-volyk commented Apr 13, 2023

View reviewed changes

nadiia-volyk requested a review from DmytroMuravskyi April 13, 2023 13:50

wynged requested review from wynged and removed request for DmytroMuravskyi April 13, 2023 14:38

wynged requested changes Apr 13, 2023

View reviewed changes

DmytroMuravskyi reviewed Apr 14, 2023

View reviewed changes

ikeough added this to the 2.0 milestone Apr 15, 2023

Refactoring

a1e3477

nadiia-volyk commented Apr 17, 2023

View reviewed changes

DmytroMuravskyi approved these changes Apr 17, 2023

View reviewed changes

wynged approved these changes Apr 17, 2023

View reviewed changes

wynged merged commit 4bc172b into hypar-io:master Apr 17, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add group name to WeightModifier. Add ability to use custom factor ag… #963

Add group name to WeightModifier. Add ability to use custom factor ag… #963

nadiia-volyk commented Apr 12, 2023 •

edited by andrewheumann

DmytroMuravskyi left a comment

DmytroMuravskyi Apr 13, 2023

DmytroMuravskyi left a comment

nadiia-volyk left a comment

wynged Apr 13, 2023

wynged Apr 13, 2023

wynged Apr 13, 2023

DmytroMuravskyi left a comment

DmytroMuravskyi left a comment

nadiia-volyk left a comment

wynged left a comment

Add group name to WeightModifier. Add ability to use custom factor ag… #963

Add group name to WeightModifier. Add ability to use custom factor ag… #963

Conversation

nadiia-volyk commented Apr 12, 2023 • edited by andrewheumann

DmytroMuravskyi left a comment

Choose a reason for hiding this comment

DmytroMuravskyi Apr 13, 2023

Choose a reason for hiding this comment

DmytroMuravskyi left a comment

Choose a reason for hiding this comment

nadiia-volyk left a comment

Choose a reason for hiding this comment

wynged Apr 13, 2023

Choose a reason for hiding this comment

wynged Apr 13, 2023

Choose a reason for hiding this comment

wynged Apr 13, 2023

Choose a reason for hiding this comment

DmytroMuravskyi left a comment

Choose a reason for hiding this comment

DmytroMuravskyi left a comment

Choose a reason for hiding this comment

nadiia-volyk left a comment

Choose a reason for hiding this comment

wynged left a comment

Choose a reason for hiding this comment

nadiia-volyk commented Apr 12, 2023 •

edited by andrewheumann