replicahq / doppelganger

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

Listbalacer update #38

Closed nikisix closed 6 years ago

nikisix commented 6 years ago

Listbalancer edge-case fix. Logging update for parallel and longer runs.


This change is Reviewable

kaelgreco commented 6 years ago

Cool! nice work. a couple tiny things below, but looking good


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


doppelganger/listbalancer.py, line 8 at r1 (raw file):

import cvxpy as cvx
import numpy as np
import logging

pep8 nit:

"Imports should be grouped in the following order: standard library imports related third party imports local application/library specific imports You should put a blank line between each group of imports."


doppelganger/listbalancer.py, line 171 at r1 (raw file):

                break
            mu = np.where(mu > 10, mu - 10, 1)
            logging.info('Solver error encountered. Importance weights have been relaxed.')

This will get logged every loop that's unsolvable now


doppelganger/scripts/download_allocate_generate.py, line 112 at r1 (raw file):


    if not os.path.exists(household_path) or not os.path.exists(person_path):
        logging.info('Data not found at: \n{}\n or {}. Downloading data from the db'

I'd suggest doing old style string formatting for logging logging.info('This is an %s %s', 'info', 'message')

"the logging package pre-dates newer formatting options such as str.format() and string.Template.".


Comments from Reviewable

nikisix commented 6 years ago

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


doppelganger/listbalancer.py, line 171 at r1 (raw file):

Previously, kaelgreco wrote…
This will get logged every loop that's unsolvable now

True. I didn't add this line in though (git blames me for it only because I removed a redundant newline from it). I can take it out or move it if you'd rather log in a different way?


doppelganger/scripts/download_allocate_generate.py, line 112 at r1 (raw file):

Previously, kaelgreco wrote…
I'd suggest doing old style string formatting for logging `logging.info('This is an %s %s', 'info', 'message')` ["the logging package pre-dates newer formatting options such as str.format() and string.Template."](https://docs.python.org/2/howto/logging.html).

will do. is it weird that format works?


Comments from Reviewable

nikisix commented 6 years ago

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


doppelganger/listbalancer.py, line 8 at r1 (raw file):

Previously, kaelgreco wrote…
pep8 nit: "Imports should be grouped in the following order: standard library imports related third party imports local application/library specific imports You should put a blank line between each group of imports."

Thanks for the tip!


Comments from Reviewable

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.4%) to 80.156% when pulling b5c2149fa6e54ff065016ae51d4053900fdb8ecd on listbalacer-update into cdbbc8ebedcf9f444b1ff764ddcf1965a6a2f508 on master.

kaelgreco commented 6 years ago
:lgtm:

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


doppelganger/listbalancer.py, line 171 at r1 (raw file):

Previously, nikisix (niki six) wrote…
True. I didn't add this line in though (git blames me for it only because I removed a redundant newline from it). I can take it out or move it if you'd rather log in a different way?

I think it's fine. we can see if it gets annoying in practice, but it seems useful.


doppelganger/scripts/download_allocate_generate.py, line 112 at r1 (raw file):

Previously, nikisix (niki six) wrote…
will do. is it weird that format works?

yeah, it is weird! was news to me until recently


Comments from Reviewable

nikisix commented 6 years ago

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


doppelganger/listbalancer.py, line 171 at r1 (raw file):

Previously, kaelgreco wrote…
I think it's fine. we can see if it gets annoying in practice, but it seems useful.

It's been useful in my runs so far to see which pumas need looser constraints than others.


Comments from Reviewable