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

truncated regression example #30

Merged
merged 17 commits into from
Feb 21, 2021

Conversation

drbenvincent
Copy link
Contributor

Ok. So this is a fresh (second) attempt at a pull request. I've added a single truncated regression example which I put into a new branch. Hopefully the pull request into main is the right thing to do?

At the moment the notebook does include quite a lot of warnings generated by theano. I'm not sure how to remove them. If someone can let me know then I can update.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@MarcoGorelli MarcoGorelli self-requested a review January 24, 2021 18:29
@ricardoV94
Copy link
Member

ricardoV94 commented Jan 24, 2021

The theano warnings will be solved in the next release, so don't worry about them for now. We can wait / rerun the notebook later to hide them :)

@ricardoV94
Copy link
Member

Do you want to include the censored regression example in the same notebook? It would be really nice to have a pointer that clearly describes the two types of models (truncated and censored) and shows them side-by-side.

@drbenvincent
Copy link
Contributor Author

Sure thing. Happy to do that. It might make it less repetitive to have it all done in one notebook.

@AlexAndorra
Copy link
Collaborator

Good idea, let's do that 👌

@drbenvincent
Copy link
Contributor Author

drbenvincent commented Jan 25, 2021

The notebook now covers both censoring and truncation. I've also added more text explanation. Let me know what you think.

Copy link
Collaborator

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great and well explained, thanks @drbenvincent 🤩
I spotted some typos and left some suggestions below -- feel free to ask if anything is unclear.
Note that CI is failing because you didn't run the pre-commit steps locally. You can take a look at the PyMC NB style guide for this 😉

@@ -0,0 +1,1356 @@
{
Copy link
Collaborator

@AlexAndorra AlexAndorra Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Same comments as for first model
  • If I understood correctly, the left_censored and right_censored steps could also just be equalities, right? left_censored = (y == bounds[0]), since y can't, by definition, be below/above the bound


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it might be safer, numerically. But can use equality if preferred

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see! No it's ok, but maybe just add this precision in a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -0,0 +1,1356 @@
{
Copy link
Collaborator

@AlexAndorra AlexAndorra Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can add some pointers to useful resources, for people who are interested?

And also maybe a high level idea of what you would do to get better estimates here on your truncated regression -- and what you mean by "a number of factors"?


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added text about the factors influencing the degree of bias.

Yet to add some pointers to further reading...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now added some info at the end of the notebook on further topics and further reading

@drbenvincent
Copy link
Contributor Author

drbenvincent commented Jan 25, 2021

I spotted some typos and left some suggestions below -- feel free to ask if anything is unclear.

Thanks @AlexAndorra. I'm new to ReviewNB. Do I accept/reject these in ReviewNB? Or is that more for discussion then I go implement the changes locally?

Note that CI is failing because you didn't run the pre-commit steps locally. You can take a look at the PyMC NB style guide for this 😉

Will take a look.

@AlexAndorra
Copy link
Collaborator

is that more for discussion then I go implement the changes locally?

Yes exactly, accepting changes isn't available in ReviewNB -- that'd be a great feature though!

@drbenvincent
Copy link
Contributor Author

I believe all that's left is to add some 'further reading' type notes at the end. I'll also clarify that you can also do censored models that impute the censored values. Should be able to finish off in the next day or so.

@drbenvincent
Copy link
Contributor Author

I think I've addressed all the points now - this is feasibly ready :) Let me know if there are any other final points.

@review-notebook-app
Copy link

review-notebook-app bot commented Jan 26, 2021

View / edit / reply to this conversation on ReviewNB

ricardoV94 commented on 2021-01-26T15:55:14Z
----------------------------------------------------------------

Small suggestion: Remove the repeated "Hopefully", I would keep the second


drbenvincent commented on 2021-01-26T16:50:22Z
----------------------------------------------------------------

done

@review-notebook-app
Copy link

review-notebook-app bot commented Jan 26, 2021

View / edit / reply to this conversation on ReviewNB

ricardoV94 commented on 2021-01-26T15:55:15Z
----------------------------------------------------------------

Suggestion: Link to our notebook on censored data in survival analysis, where there is one example of inputing the censored values: https://docs.pymc.io/notebooks/censored_data.html#Model-1---Imputed-Censored-Model-of-Censored-Data


drbenvincent commented on 2021-01-26T16:50:32Z
----------------------------------------------------------------

done :)

@ricardoV94
Copy link
Member

@drbenvincent This notebook is really great. I commented two small suggestions but feel free to ignore them :)

Copy link
Contributor Author

done


View entire conversation on ReviewNB

Copy link
Contributor Author

done :)


View entire conversation on ReviewNB

Copy link
Contributor Author

I've implemented those changes - fingers crossed that's everything. Thanks for the tips and suggestions by the way.

@review-notebook-app
Copy link

review-notebook-app bot commented Jan 26, 2021

View / edit / reply to this conversation on ReviewNB

ricardoV94 commented on 2021-01-26T16:55:11Z
----------------------------------------------------------------

Typo: preactically -> practically


drbenvincent commented on 2021-01-26T16:58:45Z
----------------------------------------------------------------

my spelling is awful! I need to see if there's a spell checking extension for jupyter lab. But resolved it now

@review-notebook-app
Copy link

review-notebook-app bot commented Jan 26, 2021

View / edit / reply to this conversation on ReviewNB

ricardoV94 commented on 2021-01-26T16:55:11Z
----------------------------------------------------------------

Typo? There are also (followed by nothing)


drbenvincent commented on 2021-01-26T16:58:10Z
----------------------------------------------------------------

done. Well spotted

Copy link
Contributor Author

done. Well spotted


View entire conversation on ReviewNB

Copy link
Contributor Author

my spelling is awful! I need to see if there's a spell checking extension for jupyter lab. But resolved it now


View entire conversation on ReviewNB

@drbenvincent
Copy link
Contributor Author

Think this is ready now :)

I have to get some teaching and other duties out the way, but I'm happy to take a look at improving the Censored Data notebook in the near future

Copy link
Collaborator

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great, thanks @drbenvincent ! I think we should wait for the new Theano-PyMC (or PyMC patch release 3.11.1) to merge -- that'll get rid of the Theano warning flood

@AlexAndorra AlexAndorra added documentation don't merge enhancement New notebook planned to be added to the collection labels Jan 27, 2021
@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

MarcoGorelli commented on 2021-01-28T18:03:51Z
----------------------------------------------------------------

This might just be me not understanding, but I find this a bit confusing - the lower bound is a horizontal line, so what does it mean to be "to the left of the lower bound"?


@fonnesbeck
Copy link
Member

@MarcoGorelli I think it is referring to "left" on the number line, rather than a plot.

@drbenvincent
Copy link
Contributor Author

@MarcoGorelli I think it is referring to "left" on the number line, rather than a plot.

Yep. I've used both the left/right and upper/lower terminology. Happy to elaborate on that in the notebook if you think it needs it. Just let me know.

@MarcoGorelli
Copy link
Contributor

It's probably clear enough to most people reading, thanks for explaining

Great job on this really nice notebook!

@drbenvincent
Copy link
Contributor Author

I'll see if I can contribute some more example notebooks in the future :) If I have an idea, should I open an issue to run it past people, or just go for it with another pull request?

@twiecki
Copy link
Member

twiecki commented Jan 29, 2021

@drbenvincent You can just write it up 👍.

@ricardoV94
Copy link
Member

Pymc 3.11.1 is out. @drbenvincent Do you want to rerun the NB in the latest version to get rid of those annoying warnings?

@drbenvincent
Copy link
Contributor Author

That should hopefully be it @ricardoV94

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks a lot @drbenvincent and looking forward for your next PRs :D

@drbenvincent
Copy link
Contributor Author

Is there anything else I need to do to get it live on the site?

@ricardoV94
Copy link
Member

Not really. I don't have permissions to merge on this repo, but someone will soon and then rebuild the docs.

@AlexAndorra
Copy link
Collaborator

All good now! Thanks for the reminder @drbenvincent and for this great PR -- as @ricardoV94, I'm looking forward to the next ones now 🤩

@AlexAndorra AlexAndorra merged commit a7d6e5c into pymc-devs:main Feb 21, 2021
@drbenvincent drbenvincent deleted the truncated-regression-example branch February 22, 2021 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
don't merge enhancement New notebook planned to be added to the collection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants