-
-
Notifications
You must be signed in to change notification settings - Fork 393
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 from_xyz functions #490
Conversation
arviz/data/io_netcdf.py
Outdated
@@ -39,3 +39,7 @@ def save_data(data, filename, *, group="posterior", coords=None, dims=None): | |||
""" | |||
inference_data = convert_to_inference_data(data, group=group, coords=coords, dims=dims) | |||
return inference_data.to_netcdf(filename) | |||
|
|||
|
|||
from_netcdf = load_data # pylint: disable=invalid-name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just rename the functions rather than alias them and have two of the same function in the API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could. Then we only need to mention this change in "what's changed". Or deprecate old functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for deprecate old functionality. If we want to be safe we can wrap load_data and add a warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm fine with deprecation, or using the fact that we're not 1.0 yet to just change it (starting up a RELEASE_NOTES.md is a good idea!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for smoothing stuff out -- looks good to me, other than canyon289's comments.
@@ -10,7 +10,7 @@ class PyMC3Converter: | |||
"""Encapsulate PyMC3 specific logic.""" | |||
|
|||
def __init__( | |||
self, *_, trace=None, prior=None, posterior_predictive=None, coords=None, dims=None | |||
self, *, trace=None, prior=None, posterior_predictive=None, coords=None, dims=None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for catching this :/
arviz/data/io_netcdf.py
Outdated
@@ -39,3 +39,7 @@ def save_data(data, filename, *, group="posterior", coords=None, dims=None): | |||
""" | |||
inference_data = convert_to_inference_data(data, group=group, coords=coords, dims=dims) | |||
return inference_data.to_netcdf(filename) | |||
|
|||
|
|||
from_netcdf = load_data # pylint: disable=invalid-name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm fine with deprecation, or using the fact that we're not 1.0 yet to just change it (starting up a RELEASE_NOTES.md is a good idea!)
Based off the comments above I'm strongly in favor of
I know we're not in 1.0 release, but we should to replace globally anyway, and it would be nice to warn out existing users, including us, that we need to stop using I do realize global replacement is painful but I'm happy to do it, if we can wait a couple of weeks. I'm also fine merging this PR and then I follow up with my suggestions in another PR if that's what we choose. |
I can do it later today. |
This looks good to me! |
Use
kw1=None, *, kw2=None, ...
in all of thefrom_xyz
functions.Each library can still use their nomenclature if needed.
renamed
load_data
(from_netcdf
) andsave_data
(to_netcdf
). Old function names are now wrappers for their new name, and raise DeprecationWarning if used.