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

Add prediction type to return the mean, variance, and mode #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

neverfox
Copy link

@neverfox neverfox commented Jun 28, 2023

The mode is only included if the underlying distribution implements it (e.g. zero-inflated does not). Additionally, this change avoids unnecessary sampling if the prediction type doesn't require it. Raises a ValueError for invalid prediction type for Expectile distribution.

@StatMixedML
Copy link
Owner

@neverfox Thanks for the PR.

Before merging, can you please

  • include try also for the mean and variance in case one or all of them are not available
  • rename properties and pred_props to moments and pred_moments, also in the docstrings, since this is more understandable
  • add an example notebook, e.g., for the Gaussian and include the properties in the prediction section

Copy link
Owner

@StatMixedML StatMixedML left a comment

Choose a reason for hiding this comment

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

@neverfox Thanks for the PR.

Before merging, can you please

  • include try also for the mean and variance in case one or all of them are not available
  • rename properties and pred_props to moments and pred_moments, also in the docstrings, since this is more understandable
  • add an example notebook, e.g., for the Gaussian and include the properties in the prediction section

@neverfox
Copy link
Author

No problem. I think it cleaner at this point to move the logic to a get_moments method, since it's starting to take over the body of predict_dist. Do you agree? Also, would you prefer it to just return None if there's nothing to do (as you did with draw_samples) or continue with the ValueError approach?

@StatMixedML
Copy link
Owner

  • Yes, having a get_moments function that has the fitted distribution as an argument within the DistributionClass makes sense
  • If one of the moments is not implemented, returning None is probably better than the ValueError way

Thanks for the suggestions.

@StatMixedML
Copy link
Owner

I was wondering that if the variance or mode are not available in the distribution class, we can also infer them from the predicted samples. Can you add this maybe.

@neverfox
Copy link
Author

neverfox commented Jul 2, 2023 via email

@StatMixedML
Copy link
Owner

Sure. It might make sense to make that behavior explicit so the user knows what they're getting?

Yes, please make sure to adjust the function description to make it clear and self-contained.

@neverfox neverfox force-pushed the feature/predict-properties branch from 7d2fe63 to 17cbe81 Compare July 2, 2023 19:25
@StatMixedML
Copy link
Owner

StatMixedML commented Jul 3, 2023

Thank you for implementing the changes!

I am currently in the process of completing the unit tests in the upcoming weeks. Once they are finalized, you can modify them accordingly to include a unit test for your specific changes. We can then merge the PR.

@StatMixedML
Copy link
Owner

can you please also add unit-tests for your function, thanks.

Additionally, this change avoids unnecessary sampling if the prediction type doesn't need it.
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.

2 participants