Adding a new "IF" node that would simply return the truevalue or falsevalue based on the test condition. #11481

reddyashish · 2021-02-09T08:22:55Z

Purpose

Task: https://jira.autodesk.com/browse/DYN-1976

This PR is made from the original one by @aparajit-pratap : #11464.

Created a new if node that would create a AST output for the following DS code

[t, f][cond ? 0 : 1];

We will have to figure out a new name for this node so that it would be clear to the users and can be found easily.
Added a test to verify the functionality of this new node.
The current if node can be removed in the next major version if we feel it would not be needed.

If the input condition has a list of boolean values then the output will be replicated based on the test input value.

Declarations

Check these if you believe they are true

The codebase 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.
This PR modifies some build requirements and the readme is updated

Reviewers

@aparajit-pratap @QilongTang @mmisol @Amoursol @mjkkirschner

Amoursol · 2021-02-09T14:50:44Z

@reddyashish Is it possible for me to test this build? If so, please let me know how. Also, have you tested with Empty List and Null inputs?

reddyashish · 2021-02-09T15:02:49Z

@Amoursol Yes I have tested this with Empty lists and null values and it works as expected.

You can pull in the changes from this branch and test it locally.

Amoursol · 2021-02-09T15:10:42Z

@Amoursol Yes I have tested this with Empty lists and null values and it works as expected.

You can pull in the changes from this branch and test it locally.

Cool - I'll build and test! :)

aparajit-pratap · 2021-02-09T15:11:33Z

@Amoursol please note that this new node does not have lacing or partial function support. It differs from the old one in that the old one did have partial function support. We did not feel it was very useful for this node as the usage of the If node as a partial function seemed ambiguous. Please let us know if you have other thoughts.

Amoursol · 2021-02-09T15:14:20Z

@Amoursol please note that this new node does not have lacing or partial function support. It differs from the old one in that the old one did have partial function support. We did not feel it was very useful for this node as the usage of the If node as a partial function seemed ambiguous. Please let us know if you have other thoughts.

@aparajit-pratap - gotcha! I need to test to see if we do need Lacing or List@Level - both were direct requests from our users when requesting feedback here: https://forum.dynamobim.com/t/request-for-feedback-if-node/45257

My concern without implementing this would be we are in the same position at a later date, but I'll test to see if the use cases in that thread are covered. The two bugs that appear to be fixed in this are definitely a step in the right direction: Empty List control and various length True/False lists not having data truncated.

aparajit-pratap · 2021-02-09T15:27:46Z

If you're comparing this with the Clockwork node, I'd like to add that it is almost identical to that. Secondly, about replication and list-at-level, in order to make this node match with user expectation of its use, we needed to disable replication for the true and false inputs (2nd and 3rd resp.). The node can only replicate on the 1st input, the test condition.

aparajit-pratap · 2021-02-09T15:46:13Z

From that forum thread to me it sounds like users are confused as to what Lacing or List@Level can do to help in the context of this node. Whatever output they expect from the node can be achieved without those additions.

aparajit-pratap · 2021-02-09T17:40:57Z

If lacing or list-at-level support is required for this node for consistency sake, I think it might be possible to still add it, however, for partial function support, I think we might need a different approach where we create a special builtin function and call that from this node:

def if_func(cond : bool, t : var[]..[], f : var[]..[])
{
    return [Imperative]
    {
        return cond ? t : f;
    }
}

Amoursol · 2021-02-09T17:57:50Z

If lacing or list-at-level support is required for this node for consistency sake, I think it might be possible to still add it, however, for partial function support, I think we might need a different approach where we create a special builtin function and call that from this node:
def if_func(cond : bool, t : var[]..[], f : var[]..[])
{
    return [Imperative]
    {
        return cond ? t : f;
    }
}

I've been busy all day so far, going to download the build and test today/tomorrow :) Just need to see what kind of data I can emulated from a messy Revit source.

aparajit-pratap · 2021-02-10T00:31:28Z

src/Libraries/CoreNodeModels/Logic/If.cs

@@ -72,4 +74,44 @@ public override IEnumerable<AssociativeNode> BuildOutputAst(List<AssociativeNode
            };
        }
    }
+
+    [NodeName("NewIf")]


Why do we need to rename it here?

We still need to decide on the final name for this node. I have just added this name as a draft. Since, the old "IF" node still exists in the Dynamo library, we will need a different name for the node.

I mean to ask if we can keep the same name for the new node while deprecating the old one.

@reddyashish @QilongTang I'm still not sure about this. To give an example, I introduced a new IntegerSlider64Bit NodeModel class but could keep the same name Integer Slider for the node. I think we may be able to do something similar here so that new If nodes have new behaviour but the same name. We might just have to figure out a way for old If nodes to continue to deserialize as the old NodeModel class.

@aparajit-pratap what about if the old IF node actually has its name migrated to "replicating if" or "legacy if" and the new node becomes "If"?

@reddyashish I think you're getting confused. The old node will be hidden from the library.

@reddyashish - AFAIK @aparajit-pratap is correct that the use of the [obsolete] attribute should hide the node - it's possible you need to use [NodeObsolete] for that though - I know there is some discrepancy between zero touch and nodeModel nodes though - so watch out for that.

Maybe a good first step is to decide what should happen?

should the original node be hidden, but still load when in an existing graph to stay backward compatible?

If a user places a new node what should it be called?

Is there any way to tell the difference between the old and new nodes in the same graph? (I assume the old ones will be in error state because of the obsolete attr - if it's working right?)

IMHO, from a user perspective they want the following

@reddyashish - AFAIK @aparajit-pratap is correct that the use of the [obsolete] attribute should hide the node - it's possible you need to use [NodeObsolete] for that though - I know there is some discrepancy between zero touch and nodeModel nodes though - so watch out for that.

Maybe a good first step is to decide what should happen?

should the original node be hidden, but still load when in an existing graph to stay backward compatible?

If a user places a new node what should it be called?

Is there any way to tell the difference between the old and new nodes in the same graph? (I assume the old ones will be in error state because of the obsolete attr - if it's working right?)

@mjkkirschner IMHO, from a user perspective:

Yes this should happen

It should simply be called "If" but reference the updated, new node.

Is the error message obvious if we do this? Or will it simply say it's missing something? Ideally, users would understand that they need to replace that node. Would Copy + Paste of that node into the same place do the trick?

I have updated the code to hide the old "If" node from the library and rename the new one to have the same name. When the users open an old graph with the if node, it will behave like the old one(conditional if). Any new "If" node that will be placed will build on the new functionality. The [NodeObsolete] attribute can only be used for a method or a property and the [obsolete] attribute will throw a warning when the old "if" class is used in any of the code. It wouldn't throw an error on the Dynamo side. Do we want to throw an error or warning for the old "if" nodes?

After discussing with Sol, we will be showing a warning message on the "If" node when an old graph is opened..

Amoursol · 2021-02-10T21:30:10Z

@reddyashish Can we get a build of Dynamo including the NewIf node? That way we get get some feedback from users prior to merge.

QilongTang · 2021-02-11T02:36:04Z

@reddyashish Can we get a build of Dynamo including the NewIf node? That way we get get some feedback from users prior to merge.

We can get a branch build out of this work for sure. Is this PR in a good state? If yes, we can merge into a feature branch and start from there

reddyashish · 2021-02-11T14:01:19Z

@Amoursol Yes, we can get the branch build like Aaron suggested. I have added tests and this PR is pending review. Just need to figure out a proper name for this name.

Amoursol · 2021-02-11T14:35:57Z

@Amoursol Yes, we can get the branch build like Aaron suggested. I have added tests and this PR is pending review. Just need to figure out a proper name for this name.

We can also crowd-source that name from the Forum with a testable build 😄 Can you let me know when you have a Branch build up?

…ame.

src/Libraries/CoreNodeModels/Logic/If.cs

QilongTang

LGTM with one comment

reddyashish · 2021-02-16T16:16:31Z

Just waiting for the self-serve to confirm no regressions.

aparajit-pratap · 2021-02-16T16:53:21Z

⚠️
I don't think we have got input from @Amoursol about 3 things for this node:

partial function support
Lacing
list-at-level
These would need more work. Also if we need partial function support, we might not be able to go with the approach in this PR ⚠️

Amoursol · 2021-02-16T16:56:59Z

⚠️
I don't think we have got input from @Amoursol about 3 things for this node:

partial function support

Lacing

list-at-level
These would need more work. Also if we need partial function support, we might not be able to go with the approach in this PR ⚠️

Can we please get a working build to let users test against? We can put that into the forum thread and get direct feedback before merging the PR, as noted above in a previous comment :)

reddyashish · 2021-02-16T17:02:01Z

@aparajit-pratap @Amoursol To get a build, we will need to merge it to a feature branch atleast. We will not be merging this to master yet as will do that after the feedback. What do you guys think?

reddyashish · 2021-02-16T17:38:10Z

Changed the base to point to a new branch "RefactoredIfNode".

Amoursol · 2021-02-16T18:48:18Z

Changed the base to point to a new branch "RefactoredIfNode".

Awesome. Are you able to create a Sandbox build for users to test please @reddyashish?

QilongTang · 2021-02-16T18:55:21Z

@reddyashish Go for it. Let me know if you have any question in terms of creating branch build after merging into feature branch

aparajit-pratap · 2021-02-16T19:56:15Z

@aparajit-pratap @Amoursol To get a build, we will need to merge it to a feature branch atleast. We will not be merging this to master yet as will do that after the feedback. What do you guys think?

Ok, I thought this was ready to be merged into master. If this is going into a feature branch for some user testing, then that sounds good.

* Adding a new "IF" node that would simply return the truevalue or falsevalue based on the test condition. (#11481) * attempt1 at creating partially applied function * change implementation of If NodeModel node * Move the functionality to a new if node that will be renamed. * Update IfTest.cs * Hiding the old If node and renaming to the new one to have the same name. * Throw a warning message when the old node is opened. * add comments. * Update the warning message and move it to the resources file. * Fix failing test Co-authored-by: Aparajit Pratap <[email protected]> * Add list level and lacing functionality to the new "If" node. (#11598) * Add lacing feature to the "If" node. * Addressing comments * additional comments * Move code into a private function. * Add tests for the new IF node. * Update one of the If tests. * Update the test. * Add the If node icon to the new node. * Changes to the Icon image * Update the IconUrl Co-authored-by: Aparajit Pratap <[email protected]>

aparajit-pratap and others added 3 commits February 6, 2021 23:43

attempt1 at creating partially applied function

724d6e9

change implementation of If NodeModel node

37ae815

Move the functionality to a new if node that will be renamed.

6200523

Amoursol self-requested a review February 9, 2021 14:50

Update IfTest.cs

3abea59

aparajit-pratap reviewed Feb 10, 2021

View reviewed changes

reddyashish added 2 commits February 16, 2021 09:33

Hiding the old If node and renaming to the new one to have the same n…

2e81153

…ame.

Throw a warning message when the old node is opened.

231f60b

QilongTang reviewed Feb 16, 2021

View reviewed changes

src/Libraries/CoreNodeModels/Logic/If.cs Outdated Show resolved Hide resolved

QilongTang reviewed Feb 16, 2021

View reviewed changes

src/Libraries/CoreNodeModels/Logic/If.cs Show resolved Hide resolved

QilongTang approved these changes Feb 16, 2021

View reviewed changes

add comments.

b9c77e6

Update the warning message and move it to the resources file.

1997c51

Fix failing test

7e43059

reddyashish changed the base branch from master to RefactoredIfNode February 16, 2021 17:36

reddyashish merged commit 218867e into DynamoDS:RefactoredIfNode Feb 16, 2021

reddyashish mentioned this pull request Jun 2, 2021

Merge if node changes #11728

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a new "IF" node that would simply return the truevalue or falsevalue based on the test condition. #11481

Adding a new "IF" node that would simply return the truevalue or falsevalue based on the test condition. #11481

reddyashish commented Feb 9, 2021 •

edited

Loading

Amoursol commented Feb 9, 2021 •

edited

Loading

reddyashish commented Feb 9, 2021

Amoursol commented Feb 9, 2021

aparajit-pratap commented Feb 9, 2021

Amoursol commented Feb 9, 2021

aparajit-pratap commented Feb 9, 2021

aparajit-pratap commented Feb 9, 2021

aparajit-pratap commented Feb 9, 2021

Amoursol commented Feb 9, 2021

aparajit-pratap Feb 10, 2021

reddyashish Feb 10, 2021

aparajit-pratap Feb 10, 2021

aparajit-pratap Feb 11, 2021

mjkkirschner Feb 11, 2021

aparajit-pratap Feb 11, 2021

mjkkirschner Feb 12, 2021

Amoursol Feb 12, 2021

reddyashish Feb 16, 2021

reddyashish Feb 16, 2021

Amoursol commented Feb 10, 2021 •

edited

Loading

QilongTang commented Feb 11, 2021

reddyashish commented Feb 11, 2021 •

edited

Loading

Amoursol commented Feb 11, 2021

QilongTang left a comment

reddyashish commented Feb 16, 2021

aparajit-pratap commented Feb 16, 2021

Amoursol commented Feb 16, 2021

reddyashish commented Feb 16, 2021

reddyashish commented Feb 16, 2021

Amoursol commented Feb 16, 2021

QilongTang commented Feb 16, 2021

aparajit-pratap commented Feb 16, 2021

Adding a new "IF" node that would simply return the truevalue or falsevalue based on the test condition. #11481

Adding a new "IF" node that would simply return the truevalue or falsevalue based on the test condition. #11481

Conversation

reddyashish commented Feb 9, 2021 • edited Loading

Purpose

Declarations

Reviewers

Amoursol commented Feb 9, 2021 • edited Loading

reddyashish commented Feb 9, 2021

Amoursol commented Feb 9, 2021

aparajit-pratap commented Feb 9, 2021

Amoursol commented Feb 9, 2021

aparajit-pratap commented Feb 9, 2021

aparajit-pratap commented Feb 9, 2021

aparajit-pratap commented Feb 9, 2021

Amoursol commented Feb 9, 2021

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

Amoursol commented Feb 10, 2021 • edited Loading

QilongTang commented Feb 11, 2021

reddyashish commented Feb 11, 2021 • edited Loading

Amoursol commented Feb 11, 2021

QilongTang left a comment

Choose a reason for hiding this comment

reddyashish commented Feb 16, 2021

aparajit-pratap commented Feb 16, 2021

Amoursol commented Feb 16, 2021

reddyashish commented Feb 16, 2021

reddyashish commented Feb 16, 2021

Amoursol commented Feb 16, 2021

QilongTang commented Feb 16, 2021

aparajit-pratap commented Feb 16, 2021

reddyashish commented Feb 9, 2021 •

edited

Loading

Amoursol commented Feb 9, 2021 •

edited

Loading

Amoursol commented Feb 10, 2021 •

edited

Loading

reddyashish commented Feb 11, 2021 •

edited

Loading