replicahq / doppelganger

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

Add helper method #13

Closed zihenglin closed 7 years ago

zihenglin commented 7 years ago

Added helper method get_combined_config_and_default_fields for combining default household fields in allocation and household fields in configuration.

Added a test file a test function for the function above.


This change is Reviewable

katbusch commented 7 years ago

Looking good! A few comments below and I'll repeat what I asked Mogeng about naming your pull request: in general we want the title of the pull request to be as descriptive as possible because it will become the commit message in the commit logs. We want others looking back at the commit history to understand which changes are where. So a little bit more info in the title could help, eg "Add helper method to config that gets all fields"


Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


doppelganger/config.py, line 78 at r1 (raw file):

    def get_combined_config_and_default_fields(allocation_default_housedold_fields,
                                               configuration_household_fields):
        """Create a tuple of combined config and the default fields.

Yay! Nice clean docs!


doppelganger/config.py, line 90 at r1 (raw file):

   default_set = set(field.name for field in allocation_default_housedold_fields)
    config_set = set(configuration_household_fields)

Since the config already has household_fields & persons_fields and the defaults are constants, it would be ideal if this method didn't take parameters but just used the DEFAULT fields & the self.household_fields and self.person_fields

Can you modify this to have two methods, something like all_person_fields and all_household_fields that don't take parameters and aren't @staticmethod?


test/test_config.py, line 25 at r1 (raw file):

            field.name for field in allocation.DEFAULT_HOUSEHOLD_FIELDS).union(
                set(configuration_household_fields))))
        np.testing.assert_equal(combined_set, correct_set)

Looks good! you can also do self.assertEqual so you don't have to import numpy


Comments from Reviewable

katbusch commented 7 years ago
:lgtm:

Looks good to me! Only change I'm requesting is to add the return type to your documentation.

HOWEVER, I noticed that when you merged in master it seems very upset with you. It's showing changes in reviewable from Kael's code. Feel free to ping me or Kael if you run into issues merging. Maybe next time we should stick we rebasing :)


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


doppelganger/config.py, line 90 at r3 (raw file):

Combination of allocation default person fields and configuration person fields

Can you write Returns tuple(string):


doppelganger/config.py, line 100 at r3 (raw file):

        """Create a tuple of combined config and the default household fields.

        Returns: Combination of allocation default household

Same here


Comments from Reviewable

zihenglin commented 7 years ago

Just added return types in the doc.


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


Comments from Reviewable

katbusch commented 7 years ago

Reviewed 3 of 5 files at r2, 1 of 2 files at r3, 1 of 1 files at r4. Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


Comments from Reviewable

katbusch commented 7 years ago
:lgtm:

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


Comments from Reviewable