Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DYN-1688: Fix regression with replication over list inputs with nested sublists #9559

Merged
merged 4 commits into from
Mar 14, 2019
Merged

DYN-1688: Fix regression with replication over list inputs with nested sublists #9559

merged 4 commits into from
Mar 14, 2019

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Mar 11, 2019 •
edited
Loading

Purpose

JIRA: https://jira.autodesk.com/browse/DYN-1688
This fixes a regression from 2.0 where a list containing an empty sublist as its first element followed by another list (of the same rank) did not replicate properly over a node.

image

Performance results before and after the fix for the same long-running test:
image

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. - No new failures apart from those already failing on SS-CI
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@reddyashish
@mjkkirschner

FYIs

@QilongTang @saintentropy

aparajit-pratap
aparajit-pratap commented Mar 11, 2019
View reviewed changes
src/Engine/ProtoCore/Utils/ArrayUtils.cs
@@ -132,8 +132,12 @@ public static ClassNode GetGreatestCommonSubclassForArray(StackValue array, Runt
var dsArray = runtimeCore.Heap.ToHeapObject<DSArray>(array);
foreach (var sv in dsArray.Values)
{
if(IsEmpty(sv, runtimeCore)) continue;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to add a type for an empty list. On the other hand we want to ignore it and inspect the next element of the array.

@QilongTang
Copy link
Contributor

Can we conclude on difference between this one and #9554? Are they both needed?

@aparajit-pratap
Copy link
Contributor Author

Can we conclude on difference between this one and #9554? Are they both needed?

I'm not sure about #9554. I'm not convinced it is the right fix. Since the self-serve CI is broken, I ran the language tests locally on this PR and they all pass. I will do more testing on this PR including manual testing.

@reddyashish
Copy link
Contributor

Added the tests for this. Once we verify on the self serve, we can merge this. I will be closing the other PR (#9554) related to this same issue

@reddyashish reddyashish mentioned this pull request Mar 12, 2019
Closed
7 tasks
@aparajit-pratap
Copy link
Contributor Author

Updated performance benchmarking results.

@mjkkirschner
Copy link
Member

So I interpret this perf benchmark to mean that the results are essentially the same?

@aparajit-pratap
Copy link
Contributor Author

So I interpret this perf benchmark to mean that the results are essentially the same?

Yep.

@aparajit-pratap aparajit-pratap merged commit da39519 into DynamoDS:master Mar 14, 2019
@aparajit-pratap aparajit-pratap deleted the dyn-1688 branch March 14, 2019 16:51
reddyashish pushed a commit to reddyashish/Dynamo that referenced this pull request Mar 14, 2019
@aparajit-pratap @reddyashish
DYN-1688: Fix regression with replication over list inputs with neste…
9d5c098 
…d sublists (DynamoDS#9559)

* fix regression with replication over list inputs with nested sublists

* Tests

* DS language test

* Update the test

(cherry picked from commit da39519)
@reddyashish reddyashish mentioned this pull request Mar 14, 2019
Merged
7 tasks
reddyashish added a commit that referenced this pull request Mar 14, 2019
@reddyashish
DYN-1688: Fix regression with replication over list inputs with neste…
758423f 
…d sublists (#9559) (#9573)

* fix regression with replication over list inputs with nested sublists

* Tests

* DS language test

* Update the test

(cherry picked from commit da39519)
This was referenced Apr 18, 2019
Closed
Closed
reddyashish pushed a commit to reddyashish/Dynamo that referenced this pull request May 21, 2019
@aparajit-pratap @reddyashish
Cherry-picking DynamoDS#9559 into 2.0.3
9e12eed
@reddyashish reddyashish mentioned this pull request May 21, 2019
Merged
7 tasks
QilongTang pushed a commit that referenced this pull request May 28, 2019
@reddyashish @QilongTang
Cherry-pick language changes into 2.0.3 branch for Refinery (#9730)
b3945d9 
* 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.
@mmisol mmisol mentioned this pull request May 22, 2020
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reddyashish reddyashish reddyashish left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aparajit-pratap @QilongTang @reddyashish @mjkkirschner