Error in assignment?

[benjeffery]

Whilst updating pre-commit (#207) it detected this line: tsdate/prior.py:658

Here's the surrounding snippet, it looks like a mistake:

# Add unary nodes below the altered ones, as their result is calculated
            # from the coalescent node above
            unary_descendants = set()
            for node in changed_nodes:
                children = prev_tree.children(node)
                if children is not None:
                    if len(children) == 1:
                        # Keep descending
                        node == children[0]   <-------- NOTE ==
                        while True:
                            children = prev_tree.children(node)
                            if len(children) != 1:
                                break
                            unary_descendants.add(node)
                            node = children[0]
                    else:
                        # Descend all branches, looking for unary nodes
                        for node in prev_tree.children(node):
                            while True:
                                children = prev_tree.children(node)
                                if len(children) != 1:
                                    break
                                unary_descendants.add(node)
                                node = children[0]

[awohns]

Thanks @benjeffery, @hyanwong would know best, but that looks ok to me, since it checks that children is of length 1 before running children[0]?

[benjeffery]

It's not children[0] that is the issue, it is the == making the line a noop!

[awohns]

Oh! Yes good catch

[benjeffery]

Interestingly the tests still pass if I replace it with = so I wonder what the performance effect is.

[benjeffery]

Looking at the code, I don't think it will change much as in the inner while loop there is a descent.

[benjeffery]

@hyanwong @jeromekelleher Any thoughts as to whether this line should be deleted or fixed to be an assignment rather than a comparison?

[jeromekelleher]

I'm not familiar with how the code works, but clearly any noops should be removed.

[hyanwong]

Just looking at it now.

[awohns]

Any thoughts @hyanwong ?

[benjeffery]

I'm happy to do the release you mentioned @awohns once this is wrapped up. I'll prod @hyanwong.

[hyanwong]

Sorry, deep in the ARG paper.

I would like to fix the node metadata struct before a release, as this would be a breaking change. See also tskit-dev/tsinfer#642

[benjeffery]

@hyanwong Sorry to bump, but would be good to close this out. I think the line can just be deleted?

[benjeffery]

I've merged #207 as it isn't a behavior change. Close this issue if you're happy that nothing else needs to change @hyanwong.

[hyanwong]

I think we can simply remove the line. If we change to an assignment then we wouldn't allocate the current node (if unary) to unary_descendants.

This is all slightly moot, as we don't usually allow dating of tree sequences with unary nodes anyway!