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.

Sign up for GitHub

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

Jump to bottom

Rename api for plots and io #248

Merged
merged 7 commits into from
Sep 19, 2018
Merged

Rename api for plots and io #248

merged 7 commits into from
Sep 19, 2018

Conversation

ahartikainen
Copy link
Contributor

io contains now

io_pymc3.py
    from_pymc3 --> InferenceData
io_pystan.py
    from_pystan --> InferenceData

convert_to_inference_data now contains **kwargs and they are passed for from_pymc3 and from_pystan functions.

All the plots are imported to plots.__init__ as

from xyzplot import xyzplot as plot_xyz

This will keep the names of the files and everything normal.

@ahartikainen
Copy link
Contributor Author

Is there something we need to fix for the doc?

@ColCarroll
Copy link
Member

I wouldn't worry about that here - the API doc will pick it up automatically, but all the examples will need to change.

@ColCarroll
Copy link
Member

maybe a sphinx directive too, but i can wrestle with that.

ColCarroll
ColCarroll reviewed Sep 18, 2018
View reviewed changes
Copy link
Member

@ColCarroll ColCarroll left a comment

Choose a reason for hiding this comment

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

I like this - thanks for being so thorough. If it is too much to change the plot file names and plot function names, feel free to merge, and we can do that in a followup.

arviz/plots/__init__.py Outdated
from .khatplot import khatplot
from .ppcplot import ppcplot
from .violintraceplot import violintraceplot
from .autocorrplot import autocorrplot as plot_autocorr
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename the files and functions as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we can.

It will affect plots/init only.

Copy link
Member

@canyon289 canyon289 Sep 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Can we leave the modulenames/filenames the same? I feel like renaming the functions should be sufficient.

@ColCarroll ColCarroll mentioned this pull request Sep 18, 2018
Merged
@canyon289 canyon289 mentioned this pull request Sep 19, 2018
Closed
canyon289
canyon289 reviewed Sep 19, 2018
View reviewed changes
arviz/plots/__init__.py Outdated
from .densityplot import densityplot as plot_density
from .energyplot import energyplot as plot_energy
from .forestplot import forestplot as plot_forest
from .kdeplot import kdeplot as plot_kde, _fast_kde, _fast_kde_2d
Copy link
Member

@canyon289 canyon289 Sep 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If were exposing _fast_kde, and _fast_kde_2d for users we should remove the underscore to make it obvious its meant for end users.

If we don't intend for users to use this method we should remove it from the import

canyon289
canyon289 reviewed Sep 19, 2018
View reviewed changes
arviz/data/convert/io_pystan.py
def pystan_to_inference_data(*, fit=None, prior=None, posterior_predictive=None,
observed_data=None, log_likelihood=None, coords=None, dims=None):
"""Convert pystan data into an InferenceData object."""
def from_pystan(*, fit=None, prior=None, posterior_predictive=None,
Copy link
Member

Choose a reason for hiding this comment

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

This documentation is great. Thank you!

@canyon289
Copy link
Member

Thank you for getting this renaming started!

@ColCarroll
Copy link
Member

Felt guilty for merging my changes, and then got a little caught up renaming the plot functions, and now I feel bad that #249 will need to be rebased, but I am now happy with these changes. Looking at other PR now.

@canyon289
Copy link
Member

canyon289 commented Sep 19, 2018 via email

canyon289
canyon289 reviewed Sep 19, 2018
View reviewed changes
arviz/plots/__init__.py
from .densityplot import plot_density
from .energyplot import plot_energy
from .forestplot import plot_forest
from .kdeplot import plot_kde, _fast_kde, _fast_kde_2d
Copy link
Member

Choose a reason for hiding this comment

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

Are these supposed to be private at the in init.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think they should be, because they are not meant for the average user.

I think they still propagate to main namespace?

Copy link
Member

Choose a reason for hiding this comment

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

Yes but only because we import them into init.py I believe. I think if we don't import them here they won't propagate to the main namespace.

In either case I'm not too worried about it now and don't want to block this for something this minor, especially since "private" isn't even a real thing in Python.

@canyon289 canyon289 merged commit f4707fe into master Sep 19, 2018
@ahartikainen ahartikainen deleted the rename_api branch September 19, 2018 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@canyon289 canyon289 canyon289 left review comments

@ColCarroll ColCarroll ColCarroll left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ahartikainen @ColCarroll @canyon289