replicahq / doppelganger

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

Pomegranate Compatibility: Frozenset -> Tuple #31

Closed nikisix closed 6 years ago

nikisix commented 6 years ago

Found an issue with the bayes net structure that arose from a change in #22. Reverting back to tuples for compatibility with pomegranate.


This change is Reviewable

katbusch commented 6 years ago

Thanks @nikisix! This also fixes #30, right?

Can you investigate whether we can cover this bug in our tests so that we don't accidentally reintroduce it?

katbusch commented 6 years ago

Awesome, thanks for fixing this and for writing the test! I've requested a couple small changes as cleanup!


Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


README.md, line 23 at r2 (raw file):

pip install .

Or for developmers

Typo


test/test_bayes_net.py, line 202 at r2 (raw file):

        structure = bayesnets.define_bayes_net_structure(nodes, edges)
        self.assertSequenceEqual(structure, ((), (0,), (0, 1)))
        #self.assertSequenceEqual(structure, (frozenset([]), frozenset([0]), frozenset([0, 1])))

Can you delete this commented out line?


test/test_bayes_net.py, line 215 at r2 (raw file):

            person_training_data, self._person_fields(), segmenter=self._person_segmenter()
        )

Can you delete this trailing whitespace?


test/test_bayes_net.py, line 225 at r2 (raw file):

Previously, nikisix wrote…
Wasn't sure about exactly which logical condition would be best to check for. Important thing is that the train call to pomegranate doesn't crash.

Looks good! It is certainly unusual but your comment explains why


Comments from Reviewable

katbusch commented 6 years ago

Looks like the linter is still mad:

./test/test_bayes_net.py:204:63: W291 trailing whitespace ./test/test_bayes_net.py:227:5: E303 too many blank lines (2)

Should just be flake8 to run it locally


Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


Comments from Reviewable

katbusch commented 6 years ago
:lgtm:

Thanks @nikisix!


Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


Comments from Reviewable