Conversation
GabrielAzevedoFerreiraQB
left a comment
GabrielAzevedoFerreiraQB
left a comment
There was a problem hiding this comment.
We're in the right direction, but there I'd say the still some good amount of things to focus on before it gets merged.
Let me know if you need time to chat/PS about it. Thanks!
causalnex/network/network.py
Outdated
| Base class for Dynamic Bayesian Network (DBN), a probabilistic weighted DAG where nodes represent variables, | ||
| edges represent the causal relationships between variables. | ||
|
|
||
| ``DynamicBayesianNetwork`` stores nodes with their possible states, edges and | ||
| conditional probability distributions (CPDs) of each node. | ||
|
|
||
| ``DynamicBayesianNetwork`` is built on top of the ``StructureModel``, which is an extension of ``networkx.DiGraph`` | ||
| (see :func:`causalnex.structure.structuremodel.StructureModel`). | ||
|
|
||
| In order to define the ``DynamicBayesianNetwork``, users should provide a relevant ``StructureModel``. | ||
| Once ``DynamicBayesianNetwork`` is initialised, no changes to the ``StructureModel`` can be made | ||
| and CPDs can be learned from the data. | ||
|
|
||
| The learned CPDs can be then used for likelihood estimation and predictions. | ||
|
|
There was a problem hiding this comment.
looks good.
I think the text should be a bit different from the one in the BN class though. It's ok to keep the similar points, but I would rather say that a DBN is a BN with the time domain taken into account, and it does X and Y that a normal BN doesn't do
WDYT?
There was a problem hiding this comment.
yes i think i need a PS here :)
causalnex/network/network.py
Outdated
| Raises: | ||
| ValueError: If the structure is not a connected DAG. | ||
| """ | ||
| super().__init__(structure) No newline at end of file |
There was a problem hiding this comment.
Just wanted to know if you're clear on the changes that will need to come here. If not let's have a PS anytime :)
There was a problem hiding this comment.
yes i think i need a PS here :)
| def checkargs(function): | ||
| def _f(*arguments, **attr): | ||
| for index, argument in enumerate(inspect.getfullargspec(function)[0]): |
There was a problem hiding this comment.
This is really great and helpful!
Could you add docs - I try to add docs every time a function does something non-obvious
Also:
- will it work for in case the user provides
attrisntead of arguments? - I usally use (*args, **kw_args), but not sure if it is the de-facto standard. what do you think?
There was a problem hiding this comment.
I believe if a dictionary is passed as an arg it is absorbed intoattr. I should test if someone calls the function like add_node(dnode=node), i'm not sure if that would be a member of argmuments or attr.
| if len(arg) == 3: | ||
| if not all(isinstance(n, DynamicStructureNode) for n in arg[:-1]): | ||
| raise TypeError("{} is not of type {}".format(arguments[index], function.__annotations__[argument])) |
There was a problem hiding this comment.
I think with docstrings it will be clearer, but would love to understand the checks themselves better.
I wonder why len(arg) == 3 - I'm sure there is a reason, just couldn't get it yet
There was a problem hiding this comment.
Its for add_weighted_edges_from when the third element of the tuple is a number
| """ | ||
| Get the subgraph with the specified node. | ||
|
|
There was a problem hiding this comment.
I also see that some functions are pretty similar to the StructureNode counterpart. Maybe we can reuse them/avoid repeating code?
There was a problem hiding this comment.
Here i think after calling
node_name = node.get_node_name()
I could call
super.get_target_subgraph(node_name)
causalnex/structure/dynotears.py
Outdated
| ( | ||
| _format_name_from_pandas(idx_col, u), | ||
| _format_name_from_pandas(idx_col, v), | ||
| DynamicStructureNode(idx_col[int(u[0])], u[-1]), # _format_name_from_pandas(idx_col, u), idx_col[int(u[0])] |
There was a problem hiding this comment.
Can we delete the comment, please? :)
2 participants
Motivation and Context
Why was this PR created?
This PR is created to add a DynamicStructureNode, DynamicStructureModel and DynamicBayesianNetwork for prediction and inference with DYNOTEARS
How has this been tested?
What testing strategies have you used?
Test driven development
Checklist
RELEASE.mdfileNotice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":
I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.
I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.