bioFAM / MOFA

Multi-Omics Factor Analysis
GNU Lesser General Public License v3.0
235 stars 60 forks source link

Do added Markov Blankets get passed over? #36

Closed vsomnath closed 5 years ago

vsomnath commented 5 years ago

https://github.com/bioFAM/MOFA/blob/131593ab00e0d43234ba096277d21e7a8d0f1d8e/mofapy/core/build_model.py#L131

Here, the nodes argument is given as nodes=init.getNodes(). If you look at the lines above it goes as:

nodes = init.getNodes()
# Series of steps adding Markov Blankets
bayes_net = BayesNet(dim=dim, nodes=init.getNodes(), options=train_opts)

The changes made to the Markov Blanket, are not passed on to the BayesNet when created. Is this the intended behavior?

rargelaguet commented 5 years ago

Hi Vignesh,

The variable nodes is created by reference, not by deep copy.

nodes = init.getNodes()

Hence, every time we modify the Markov blankets we are doing this by reference, and when calling init.getNodes() you should get (by reference) the nodes with the updated Markov blanket.

Having said that, I agree is a bit misleading for the reader and I should do this instead:

bayes_net = BayesNet(dim=dim, nodes=nodes, options=train_opts)

Thanks for the catch, I didn't have time yet to clean some parts of the Python code, as I wasn't expecting anyone to dig into it. But hopefully won't be too messy :)

vsomnath commented 5 years ago

Thanks for the clarification Richard!

I'm using MoFA and other multi-omics integration methods as part of a benchmark project.

The Python version is extremely readable and very easy to follow (thank you for that!) but there were some places which had Java style camelCase and some with Python underscore. I fixed that as I read along though.

rargelaguet commented 5 years ago

Sounds good, if you have questions regarding the interpretation or the results, feel free to contact me using the Slack group (link in the README.md).