Skip to content

Conversation

@mscroggs
Copy link
Collaborator

@mscroggs mscroggs commented May 1, 2025

Implement a do algorithms that takes a graph and replaces a node with a constant ParameterNode and removes any edges going into that node.

As expectation and other algorithms already only iterate over ancestors of output nodes, there's no need to trim the graph further when applying a do.

Builds on #39, so is currently set to merge into that branch.

@mscroggs mscroggs changed the base branch from main to mscroggs/parameter-node May 1, 2025 06:45
Base automatically changed from mscroggs/parameter-node to main May 1, 2025 08:35
@willGraham01
Copy link
Collaborator

Pre-emptive note here: in #40 the ParameterNodes are always assumed to have values that will vary (see how parameter nodes are gathered here).

To avoid the solver accidentally picking up these constant parameters as values that should vary we could either:

  • ParameterNodes have another internal is_constant flag, that defaults to False but can be set to True. Then the aforementioned line checks if node.is_parameter and not node.is_constant.
  • ConstantNode class like was suggested before (but I think that's overkill since it would be almost identical to ParameterNode).
  • Have the do operator edit the DistributionNode's that now depend on the constant node, by moving the now-fixed value from parameters to constant_parameters for each affected node.

@mscroggs
Copy link
Collaborator Author

mscroggs commented May 1, 2025

  • Have the do operator edit the DistributionNode's that now depend on the constant node, by moving the now-fixed value from parameters to constant_parameters for each affected node.

Ooh, we could just remove the node that the do is setting the value of and do this move without needing to add a new node. This feels very neat so I'll do this

@mscroggs mscroggs closed this May 1, 2025
@mscroggs mscroggs reopened this May 1, 2025
@willGraham01 willGraham01 self-requested a review August 4, 2025 09:20
Copy link
Collaborator

@willGraham01 willGraham01 left a comment

Choose a reason for hiding this comment

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

I think the new_nodes dictionary in the do method is being ignored? Or it doesn't need to exist perhaps? Either way I think I get what it's meant to be doing.

With that fix - and some slightly more verbose language - I think do should be do-ing what we expect 🥳 Have added some suggestions for what to test regarding this method - TLDR would focus on only testing the connectivity of the graph that do produces, and that the parameters have been moved to constant_parameters correctly.

mscroggs and others added 3 commits August 4, 2025 15:55
Co-authored-by: Will Graham <32364977+willGraham01@users.noreply.github.com>
Co-authored-by: Will Graham <32364977+willGraham01@users.noreply.github.com>
@mscroggs mscroggs mentioned this pull request Aug 4, 2025
2 tasks
@mscroggs
Copy link
Collaborator Author

mscroggs commented Aug 4, 2025

I think the CI will pass here once #56 is merged

@mscroggs mscroggs requested a review from willGraham01 August 5, 2025 10:54
@willGraham01 willGraham01 merged commit 526b96d into main Aug 5, 2025
5 checks passed
@willGraham01 willGraham01 deleted the mscroggs/do branch August 5, 2025 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants