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.

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

Functional Loops over Time #1471

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

FelipeGiro
Copy link
Contributor

Your checklist for this pull request

Please review the guidelines for contributing to this repository.

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
  • Make sure you are making a pull request against the dev branch (left side). Also you should start your branch off our dev.
  • Check the commit's or even all commits' message styles matches our requested structure.

Issue number(s) that this pull request fixes

List of changes to the codebase in this pull request

  • substitute the creation of the Bayesian model creation to a build-in DynamicBayesianNetwork function get_constant_bn.
  • As get_constant_bn already have the CPDs, removed add_cpds

NOTE:

Code is not fully functional. The line

start_markov_model.add_edges_from(combinations_slice_0[0])
add the edges in a different way, resulting in the following nodes:

NodeView(('A_0', 'A_1', 'B_1', 'C_0', 'B_0', 'C_1', <DynamicNode(A, 0) at 0x22298476f08>, <DynamicNode(B, 0) at 0x2229847f0c8>, <DynamicNode(C, 0) at 0x2229847f108>))

As you can see, the three last nodes were already added. I don't know the reason they are represented differently.

The question is: should we change how the node is represented/compared or should we come back to base.py and think another solution other than get_constant_bn

@ankurankan
Copy link
Member

@FelipeGiro Sorry for the late reply. I am not sure what is the best way here. I remember that I had tried to change the get_constant_bn method to return a DynamicNode object for the nodes, but that caused a lot of trouble in integrating it with parameter learning code. And I couldn't find a nice way (without a lot of changes) to make it work with parameter learning and hence resorted to changing the node names with _. I am not sure if inference would work fine with the DynamicNode objects, but if it does, I would say we can add an additional argument to get_constant_bn to return either _ nodes or DynamicNode. Else, we can completely switch to the _ node names.

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.

DynamicBayesianNetwork limitations
2 participants