search

replicahq / doppelganger

A Python package of tools to support population synthesizers
Apache License 2.0
165 stars 32 forks source link

Bayes net tests #4

Closed katbusch closed 7 years ago

katbusch commented 7 years ago

Add more coverage to bayes nets, add defensive tuple creation in bayes net update

This change is Reviewable

kaelgreco commented 7 years ago
:lgtm:

Nice! just a suggestion for a comment and clarifying question

Reviewed 3 of 3 files at r1. Review status: all files reviewed at latest revision, 2 unresolved discussions.

setup.cfg, line 2 at r1 (raw file):

[flake8]
ignore = H301

It'd be nice to add a comment here to explain what the codes are. Not a huge issue with only one.

doppelganger/bayesnets.py, line 256 at r1 (raw file):

            assert len(old) == len(new)
            for i in xrange(len(old)):
                if tuple(old[i]) != tuple(new[i]):

what does this protect against?

Comments from Reviewable

katbusch commented 7 years ago

Review status: 1 of 3 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

setup.cfg, line 2 at r1 (raw file):

Previously, kaelgreco wrote…
It'd be nice to add a comment here to explain what the codes are. Not a huge issue with only one.

Ended up removing this rule instead, it wasn't needed

doppelganger/bayesnets.py, line 256 at r1 (raw file):

Previously, kaelgreco wrote…
what does this protect against?

Numpy arrays == check return an array of bools instead of a bool. If I cast to a tuple it will work whether the user passes in arrays or tuples

Comments from Reviewable