search

HealthCatalyst / healthcareai-py

Python tools for healthcare machine learning
http://healthcare.ai
MIT License
316 stars 188 forks source link

Grain column vector must exist in order to add it to results. #453

Closed danwellisch1 closed 6 years ago

danwellisch1 commented 6 years ago

Change needed in both make_predictions and make_factors.

danwellisch1 commented 6 years ago

Nosetests passed locally. What went wrong in the Travis CI build that wouldn't have failed locally?

danwellisch1 commented 6 years ago

Yes. Will fix and update...

On Jan 19, 2018 4:46 PM, "Levi Thatcher" notifications@github.com wrote:

@levithatcher commented on this pull request.

In healthcareai/trained_models/trained_supervised_model.py https://github.com/HealthCatalyst/healthcareai-py/pull/453#discussion_r162750716 :

@@ -262,8 +266,12 @@ def make_factors(self, dataframe, number_top_features=3):

Run the raw dataframe through the preparation process

prepared_dataframe = self.prepare_and_subset(dataframe)

@danwellisch1 https://github.com/danwellisch1 Appreciate this work! Can you please fix this comment so the lines stay within 80 characters?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HealthCatalyst/healthcareai-py/pull/453#pullrequestreview-90268358, or mute the thread https://github.com/notifications/unsubscribe-auth/AgHbAdfr3yucfE8XBCEWzJN_VwveKZdzks5tMRthgaJpZM4Q8Q8b .

levithatcher commented 6 years ago

@danwellisch1 FYI I'm looking at why these tests are failing. They all pass for you?

danwellisch1 commented 6 years ago

Cool...thanks!

On Jan 31, 2018 9:21 AM, "Levi Thatcher" notifications@github.com wrote:

@danwellisch1 https://github.com/danwellisch1 FYI I'm looking at why these tests are failing.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HealthCatalyst/healthcareai-py/pull/453#issuecomment-361964711, or mute the thread https://github.com/notifications/unsubscribe-auth/AgHbAXTJSr17PGiWxnRGuOr9WDJNdIsCks5tQITigaJpZM4Q8Q8b .

levithatcher commented 6 years ago

@danwellisch1 it looks like there's a fix for your Travis CI errors in PR #467.

Am getting our Appveyor integration fixed, so we can run this through windows CI--at that point we'll

  1. merge his
  2. ask you to merge master into your branch (which is good experience to have)
  3. then run CI again

Having those CI checks running smoothly saves so much manual effort, so it'll be worth getting that appveyor fix in. Appreciate your patience and work!

danwellisch1 commented 6 years ago

Sure. I will wait for your signal.

On Jan 31, 2018 12:27 PM, "Levi Thatcher" notifications@github.com wrote:

@danwellisch1 https://github.com/danwellisch1 it looks like there's a fix for your Travis CI errors in PR #467 https://github.com/HealthCatalyst/healthcareai-py/pull/467.

Am getting our Appveyor integration fixed, so we can run this through windows CI--at that point we'll

  1. merge his
  2. ask you to merge master into your branch (which is good experience to have)
  3. then run CI again

Having those CI checks running smoothly saves so much manual effort, so it'll be worth getting that appveyor fix in. Appreciate your patience and work!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HealthCatalyst/healthcareai-py/pull/453#issuecomment-362025383, or mute the thread https://github.com/notifications/unsubscribe-auth/AgHbAcuBP7fhy27rGtRuH2gxHuW2muakks5tQLCYgaJpZM4Q8Q8b .

levithatcher commented 6 years ago

@danwellisch1 appreciate your patience. Now that @jlitzingerdev's work was merged to master, would you mind merging master into your branch?

That should fix the CI errors (and we'll see that noted here in the PR once you push again).

See here if you want help with the merge syntax. Thanks!

danwellisch1 commented 6 years ago

Levi:

So, the link you provided me shows this.:

git checkout feature1 git merge master

These commands operate locally, right? The master that gets merged is the master that is local on the copy of the forked repository danwellisch1 https://github.com/danwellisch1/healthcareai-py https://github.com/danwellisch1/healthcareai-py, so since the forked repo

I am thinking these are the steps...

  1. Merge

HealthCatalyst/healthcareai-py https://github.com/HealthCatalyst/healthcareai-py

back into

danwellisch1 https://github.com/danwellisch1/healthcareai-py https://github.com/danwellisch1/healthcareai-py

  1. Locally do a git checkout master (my local copy of danwellisch1/healthcare-py)

  2. Locally do a git merge origin/master to merge danwellisch1/healthcare-py from forked repo

    on Github back down to my local master.

  3. git checkout -b

  4. git merge master

  5. git push origin/master (push back up to danwellisch1/healthcare-py)

Let me know if I have this right and/or see if there are any shortcuts I can take in the steps. The one thing I don't know how to do is 1. So, please advised on that.

Dan

On Thu, Feb 1, 2018 at 1:49 PM, Levi Thatcher notifications@github.com wrote:

@danwellisch1 https://github.com/danwellisch1 appreciate your patience. Now that @jlitzingerdev https://github.com/jlitzingerdev's work was merged to master, would you mind merging master into your branch?

That should fix the CI errors (and we'll see that noted here in the PR once you push again).

See here https://stackoverflow.com/a/16957483 if you want help with the merge syntax. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HealthCatalyst/healthcareai-py/pull/453#issuecomment-362381190, or mute the thread https://github.com/notifications/unsubscribe-auth/AgHbAeI3fDhvmn949jxCpOx3X_Qi2tdBks5tQhUvgaJpZM4Q8Q8b .

-- [image: logo] Dan Wellisch President & Senior Data Analyst/Engineer Wellisch Software Technologies, Inc. 847-254-9355

jlitzingerdev commented 6 years ago

@danwellisch1 Assuming the healthcareai workflow matches that of most open source projects, to accomplish 1 you need to merge the upstream master into your local master. That implies your local clone will have two remotes -- your fork, and the healthcareai upstream. The following might be useful references: https://gist.github.com/Chaser324/ce0505fbed06b947d962 https://help.github.com/articles/syncing-a-fork/ https://git-scm.com/book/en/v2/Git-Basics-Working-with-Remotes

Cheers, hope this might be useful.

danwellisch1 commented 6 years ago

Thanks Jason. I think this is exactly what I need.

Dan

On Fri, Feb 2, 2018 at 11:31 PM, Jason Litzinger notifications@github.com wrote:

@danwellisch1 https://github.com/danwellisch1 Assuming the healthcareai workflow matches that of most open source projects, to accomplish 1 you need to merge the upstream master into your local master. That implies your local clone will have two remotes -- your fork, and the healthcareai upstream. The following might be useful references:

https://help.github.com/articles/syncing-a-fork/

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HealthCatalyst/healthcareai-py/pull/453#issuecomment-362781779, or mute the thread https://github.com/notifications/unsubscribe-auth/AgHbAW-b5JMnT0TsRlPkk38FrDDWzdhcks5tQ-8ngaJpZM4Q8Q8b .

-- [image: logo] Dan Wellisch President & Senior Data Analyst/Engineer Wellisch Software Technologies, Inc. 847-254-9355