DYN-1722 #9578

reddyashish · 2019-03-15T19:22:53Z

Purpose

This PR is to address a regression issue from 2.0. (https://jira.autodesk.com/browse/DYN-1722)

This also fixes most of the non-regressions from https://jira.autodesk.com/browse/DYN-1496. The only case that is failing would be the second case in the description of the above JIRA.

Explanation:
Instead of the just returning the first replication option that matches the target function, we check for all the replications that match the target function and then choose the replication option with largest rank. In the case where both cartesian and zipped replication options are possible, we check for the first option that matches the target function and modify the replication option(in next iterations) only if
a similar replication option, with a higher rank, is matched with the target function.

Performance test results on around 11 graphs:

Before the fix:

After the fix:

Declarations

Check these if you believe they are true

The code base is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@aparajit-pratap @mjkkirschner @QilongTang

test/Engine/ProtoTest/Associative/MethodResolution.cs

src/Engine/ProtoCore/Lang/CallSite.cs

mjkkirschner · 2019-03-15T20:39:55Z

@reddyashish have you had a chance to try running this change over a set of graphs before and after using the perf console app?

reddyashish · 2019-03-15T20:54:25Z

Not yet, discussed this with Aparajit and we will be doing it next. We would be needing a set of large graphs that would be testing cases involving function match resolution. Any idea on if we got any?

mjkkirschner · 2019-03-15T20:57:25Z

I am sure if we can be specific about what graph types we want @JacobSmall and @Amoursol can provide a bunch.

JacobSmall · 2019-03-16T17:47:17Z

If I don’t have one ready I can likely build one (or ten) up pretty quickly. Looking for a lot of nodes or a lot of objects?

aparajit-pratap · 2019-03-18T01:53:55Z

@reddyashish please enumerate and explain the case(s) that are yet to be fixed. You mentioned that the second example in DYN-1496 isn't fixed but you can generalize it a bit more; for example: A heterogeneous list of sublists, where the first sublist type does not match the target function, does not replicate properly. Example:

src/Engine/ProtoCore/Lang/CallSite.cs

aparajit-pratap

Ashish, I don’t fully understand the code change especially the IsCompatible function. I saw the 2 test failures you mentioned on self serve and I didn’t see why the above change was required to address them?

aparajit-pratap · 2019-03-18T20:30:50Z

src/Engine/ProtoCore/Lang/CallSite.cs

+        private Boolean IsCompatibleReplicationOption(List<ReplicationInstruction> oldOption, List<ReplicationInstruction> newOption)
+        {
+            if (oldOption.Count > 0 && newOption.Count > 0 && oldOption.Count < newOption.Count)
+            {


I’m not sure I understand this change. Why are you checking equality only for the first option?

For those 2 cases where it was failing, as we are checking for all the replication options(to find a match), the first match is found for "cartesian:indices=0" option. Then it finds a match for "zipped:indices=0,1" and it applies zipped replication option(as this is the final option that is tested). Previously, the first option that found a match was applied directly. After this change, it would not accept the zipped replication as the cartesian option has already found the match.
Another case is, we want to accept this new replication option, if it is of the higher rank than the previous option (old option: "cartesian:indices=0" and new option: "cartesian:indices=0, cartesian:indices=0". Similar option but checks for one extra depth level). Since all options are unique, I was checking the first element and the count for the new option to be higher than the first. Also can use, newOption.Count = oldOption.Count +1.

I'm definitely having a hard time understanding this one, is it possible @reddyashish you could draw a diagram or do a longer writeup of the problem here and the approach taken to fix it - I know it's kind of a tall order.

so - I'm trying to think of cases where this will fail -
I think naming this method or adding a summary might help - but my interpretation is that this method is used to find List<ReplicationInstruction> where the first instruction matches the previous one but is of a greater depth?

What I cannot figure out or explain yet is what case this is used to avoid or to guarantee we hit? Can you try to sum it up in the summary of the method or in the git here.

src/Engine/ProtoCore/Lang/CallSite.cs

mjkkirschner · 2019-03-22T01:56:06Z

src/Engine/ProtoCore/Lang/CallSite.cs

                        }
                    }
+                    if (matchFound)
+                        return;


@reddyashish so - what about case 5 - why do we exit the other cases late, and exit case 5 as soon as we find a match?

I did not understand this case and it adds an empty replication option to the previous list and finds the match. I was not able to find any examples related to this case.

I will try to find when it's called.
@saintentropy any ideas?

@mjkkirschner @reddyashish In general this whole function is still black magic to me. I have been able to look at indivual examples and how they flow through but it is hard to make judgment on individual PR's without holistic picture. I think we need to document examples of different data, data structures, replication settings, and functions and what the result replication instruction and function endpoint list. Not sure if we need to do that on this PR but it is hard to validate the code changes otherwise

For a start - I've filed a followup task to cover each of the missing cases with explicit tests.

mjkkirschner · 2019-03-22T01:56:57Z

I think these cases really need examples. I know they are not new, but I think if you have some idea about which types of function calls apply to which cases those examples inline would be a good addition to the comments.

src/Engine/ProtoCore/Lang/CallSite.cs

mjkkirschner · 2019-03-22T02:01:39Z

src/Engine/ProtoCore/Lang/CallSite.cs

-                        return;
+                        if (replicationInstructions == null || IsCompatibleReplicationOption(replicationInstructions, replicationOption))
+                        {
+                            //Otherwise we have a cluster of FEPs that can be used to dispatch the array


I find this comment confusing - what is the otherwise referring to - theres no comment before this and we are inside an if statement... not an else statement -

mjkkirschner · 2019-03-22T15:29:57Z

src/Engine/ProtoCore/Lang/Replication/ReplicationInstruction.cs

@@ -44,5 +45,22 @@ public override string ToString()
            }

        }
+
+        public Boolean Equals(ReplicationInstruction oldOption) {


this does not seem to actually perform a full equality check... it only returns true for zip replication - you should override Equals like this

https://docs.microsoft.com/en-us/dotnet/api/system.object.equals?view=netframework-4.7.2

use the override keyword

it's a struct - so @reddyashish you may want to read this:
https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/statements-expressions-operators/how-to-define-value-equality-for-a-type

@mjkkirschner Do you want me change anything in this Equals method after our discussion on friday or is this ok?

hmm, well I guess I am a bit confused why you do not just compare the properties directly? ie Why all the use of Except?

Added the comments to code, on why we can use Except to compare the elements in both the lists.

mjkkirschner · 2019-03-22T22:11:47Z

@reddyashish thanks for updating comments and tests - is the only thing left that we discussed the performance testing of a pathologically bad case? (super nested?) or many many parameter function?

reddyashish · 2019-03-25T14:38:14Z

Performance results for a case which has around 110 replication replication trials.
Before the fix(snapshot on top) and after the fix(snapshot on below):

mjkkirschner · 2019-03-25T16:09:55Z

src/Engine/ProtoCore/Lang/Replication/ReplicationInstruction.cs

+            if (this.Zipped == oldOption.Zipped)
+            {
+                if (this.ZipIndecies == null && oldOption.ZipIndecies == null)
+                    return true;


why is this true even if the zip algorithm is different?

Added the zipAlgorithm check.

mjkkirschner · 2019-03-25T16:10:32Z

src/Engine/ProtoCore/Lang/Replication/ReplicationInstruction.cs

+                if (this.ZipIndecies == null && oldOption.ZipIndecies == null)
+                    return true;
+
+                if (this.ZipIndecies != null && oldOption.ZipIndecies != null)


I'm not getting something here - can you add a comment - but like I said above, I'm confused why we're not using sequence equals or something like that?

@reddyashish explained this is an optimization.

Fastest way to compare elements of both the lists:
https://stackoverflow.com/questions/12795882/quickest-way-to-compare-two-list

I have added comments to the code.

mjkkirschner · 2019-03-25T18:12:51Z

@reddyashish - Is this ready to merge?

reddyashish · 2019-03-25T19:10:03Z

The self-serve looks good.
https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/219/

The performance benchmark tests for the graphs(created by Scott) are still running, so once that is done we can merge it.

reddyashish · 2019-03-25T20:28:07Z

Performance benchmark results for graphs in here:https://github.com/DynamoDS/Dynamo/tree/master/tools/Performance/DynamoPerformanceTests/graphs

Before these changes:

After these changes:

Both the times, the run time was higher than what Scott has reported in his PR. I followed up with Scott and he will be running them again on his machine to see if he can see any difference.

mjkkirschner · 2019-03-25T20:30:12Z

@scottmitchell - we really need to figure out how to get the full name in the performance results output 😉... @reddyashish what do you make of those results?

reddyashish · 2019-03-25T20:34:35Z

The Mean, Error, StdDev values are not that different but the run time is. That is confusing to me.
https://user-images.githubusercontent.com/43763136/54951795-e0286b00-4f1a-11e9-9df3-7f3f79ff3f3e.PNG
Also, I ran both the tests parallely. Scott told me he ran it only once, so he will be doing it again today.

mjkkirschner · 2019-03-25T20:37:14Z

if you ran the tests in parallel it makes sense they will have longer run time - it also appears that it ran them both different numbers of times in each comparison - I would look only at the benchmark times relative to each other (before and after), not the total runtime.

* DYN-1722 * Variable change name * Update the callsite.cs * Modifying the equals function. * adding comments * Test comments * Changing variable name * Adding zipAlgorithm check and comments. * More Comments. (cherry picked from commit 62e1b56)

* Cherry-picking #9388 into 2.0.3 * Cherry-picking #9559 into 2.0.3 * Cherry-picking #9578 into 2.0.3 * cherry-picking #9632 into 2.0.3 * cherry-picking #9408 into 2.0.3 * cherry-picking #9441 into 2.0.3 * Adding gradient.png for the Test_PerforationsByImage test. This was missed while cherrypicking #9441 * Removing DSCoreDataTests.cs as this was the test fixture was introduced in a different commit and is not needed here.

DYN-1722

4d1b63e

reddyashish commented Mar 15, 2019

View reviewed changes

test/Engine/ProtoTest/Associative/MethodResolution.cs Show resolved Hide resolved

mjkkirschner reviewed Mar 15, 2019

View reviewed changes

src/Engine/ProtoCore/Lang/CallSite.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Mar 15, 2019

View reviewed changes

src/Engine/ProtoCore/Lang/CallSite.cs Outdated Show resolved Hide resolved

Variable change name

63a542b

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

a3e1bab

aparajit-pratap reviewed Mar 18, 2019

View reviewed changes

src/Engine/ProtoCore/Lang/CallSite.cs Outdated Show resolved Hide resolved

src/Engine/ProtoCore/Lang/CallSite.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Mar 18, 2019

View reviewed changes

Update the callsite.cs

14877d8

mjkkirschner reviewed Mar 22, 2019

View reviewed changes

src/Engine/ProtoCore/Lang/CallSite.cs Show resolved Hide resolved

mjkkirschner reviewed Mar 22, 2019

View reviewed changes

src/Engine/ProtoCore/Lang/CallSite.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Mar 22, 2019

View reviewed changes

Modifying the equals function.

2ae85fa

mjkkirschner reviewed Mar 22, 2019

View reviewed changes

reddyashish added 2 commits March 22, 2019 14:30

adding comments

5f9fb87

Test comments

6811327

Changing variable name

64bab79

mjkkirschner reviewed Mar 25, 2019

View reviewed changes

reddyashish added 2 commits March 25, 2019 13:06

Adding zipAlgorithm check and comments.

58ec196

More Comments.

f665632

mjkkirschner added the LGTM Looks good to me label Mar 25, 2019

reddyashish merged commit 62e1b56 into DynamoDS:master Mar 25, 2019

reddyashish mentioned this pull request Mar 26, 2019

[Cherry-Pick] DYN-1722 (#9578) #9597

Merged

7 tasks

aparajit-pratap mentioned this pull request Apr 11, 2019

Remove category failure from passing replication test #9643

Merged

7 tasks

This was referenced Apr 18, 2019

Improvement suggestion: 2D lists with empty sublists triggers "Dereferencing a non-pointer" exception #8411

Closed

Bug, Dynamo 0.82: Revit nodes fail if the first item in the input list is a null or an empty list. #5238

Closed

reddyashish added a commit that referenced this pull request May 21, 2019

Cherry-picking #9578 to 2.0.3

fab0735

reddyashish added a commit to reddyashish/Dynamo that referenced this pull request May 21, 2019

Cherry-picking DynamoDS#9578 into 2.0.3

dd4e1bc

reddyashish mentioned this pull request May 21, 2019

Cherry-pick language changes into 2.0.3 branch for Refinery #9730

Merged

7 tasks

DYN-1722 #9578

DYN-1722 #9578

Conversation

reddyashish commented Mar 15, 2019 • edited Loading

Purpose

Declarations

Reviewers

mjkkirschner commented Mar 15, 2019

reddyashish commented Mar 15, 2019 • edited Loading

mjkkirschner commented Mar 15, 2019

JacobSmall commented Mar 16, 2019

aparajit-pratap commented Mar 18, 2019

aparajit-pratap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reddyashish Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Mar 22, 2019 • edited Loading

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

mjkkirschner commented Mar 22, 2019 • edited Loading

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

mjkkirschner commented Mar 22, 2019

reddyashish commented Mar 25, 2019

Choose a reason for hiding this comment

reddyashish Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Mar 25, 2019 • edited Loading

reddyashish commented Mar 25, 2019 • edited Loading

reddyashish commented Mar 25, 2019 • edited Loading

mjkkirschner commented Mar 25, 2019

reddyashish commented Mar 25, 2019 • edited Loading

mjkkirschner commented Mar 25, 2019 • edited Loading

reddyashish commented Mar 15, 2019 •

edited

Loading

reddyashish commented Mar 15, 2019 •

edited

Loading

reddyashish Mar 18, 2019 •

edited

Loading

mjkkirschner Mar 22, 2019 •

edited

Loading

mjkkirschner commented Mar 22, 2019 •

edited

Loading

reddyashish Mar 25, 2019 •

edited

Loading

mjkkirschner Mar 25, 2019 •

edited

Loading

mjkkirschner commented Mar 25, 2019 •

edited

Loading

reddyashish commented Mar 25, 2019 •

edited

Loading

reddyashish commented Mar 25, 2019 •

edited

Loading

reddyashish commented Mar 25, 2019 •

edited

Loading

mjkkirschner commented Mar 25, 2019 •

edited

Loading