Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

normalize data for dcm #209

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

normalize data for dcm #209

wants to merge 12 commits into from

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Mar 30, 2018

This is a start on #208 still to do:

  • make it optional
  • add test for both behaviors
  • add l1 l2 regularization
  • test regularization

But I thout I'd open this to get some feedback.

@Eh2406
Copy link
Contributor Author

Eh2406 commented Apr 5, 2018

I don't know why CI is failing. Help?

@Eh2406
Copy link
Contributor Author

Eh2406 commented Apr 9, 2018

AppVeyor is Failed building wheel for pandana I don't think I can fix that.
I am working on the travis things.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 93.959% when pulling d10e194 on SEMCOG:normalize-data into 2497039 on UDST:master.

@coveralls
Copy link

coveralls commented Apr 9, 2018

Coverage Status

Coverage increased (+0.06%) to 94.409% when pulling d832f16 on SEMCOG:normalize-data into 79f815a on UDST:master.

@janowicz
Copy link
Contributor

janowicz commented Apr 18, 2018

Thanks for this @Eh2406! The passing Travis builds are the relevant benchmark here. The appveyor Windows build are failing due to pandana on Windows, which is unrelated to the present PR. We'll fix that, or we may remove pandana as a hard CI dependency of core urbansim (as little in this repo specifically needs pandana).

@Eh2406
Copy link
Contributor Author

Eh2406 commented Apr 20, 2018

I don't have a lot of experience with pytest. What test would you like to see, and how do you recommend I implement them?

@Eh2406
Copy link
Contributor Author

Eh2406 commented May 1, 2018

Ping. To be clear I am stuck waiting for advice on what tests you would like to see.

@janowicz
Copy link
Contributor

janowicz commented May 2, 2018

Some thoughts (curious what others think too):
test_dcm.py

  • Create a "normalized_dcm" fixture that takes the existing basic_dcm fixture as a starting point and set values for normalized/l1/l2
  • Using the test_mnl_dcm function as a template, implement a high-level smoke-test just to see that the model with normalization can be fitted, that probabilities can be calculated from the resulting model, and that 'Normalization Mean' / 'Normalization Std' are in the resulting fit parameters.

test_mnl.py

  • Can any of the normalization/regularization changes to mnl.py be tested here? Perhaps a test for expected coefficients that are different when non-zero l1/l2 values supplied? And a smoke-test to ensure that mnl.mnl_simulate() runs with normalization_mean / normalization_std?

Eh2406 added 2 commits May 2, 2018 16:04
add a simple_dcm for the test that cannot be parameterized
@Eh2406
Copy link
Contributor Author

Eh2406 commented May 3, 2018

I parameterized basic_dcm to run with normalization and 3 different regularizations. Then looked through all the newly failing tests. This sussed out 2 places where I did not pass the new arguments in the correct order. Oops. Meny of the test can be made to pass with an if on basic_dcm.normalize. For the 3 that could not be done so easily, I added a simple_dcm that matched the old basic_dcm and switched them to use that.

Thoughts?

@Eh2406
Copy link
Contributor Author

Eh2406 commented May 24, 2018

merged and ci is green. Thoughts? What else would you like to see before accepting?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants