8000 ENH: _StringFuncParser to get numerical functions callables from strings by alvarosg · Pull Request #7464 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

ENH: _StringFuncParser to get numerical functions callables from strings #7464

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

Merged
merged 11 commits into from
Dec 15, 2016
Merged

ENH: _StringFuncParser to get numerical functions callables from strings #7464

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0ad3710
First commit
alvarosg Nov 15, 2016
509ed9f
Creation of class _FuncInfo
alvarosg Nov 15, 2016
bb042f6
Added parameter check, and other feedback from the PR
Nov 16, 2016
4fa16f3
Removed np.cbrt for compatibility with older numpy versions
alvarosg Nov 16, 2016
c5770fd
Corrections from @efiring
alvarosg Dec 13, 2016
69f2a04
Solved compatibility error with python 3+
alvarosg Dec 13, 2016
94a1af0
Feedback from @story645
alvarosg Dec 14, 2016
331d5d3
Tiny change missing @story645
alvarosg Dec 14, 2016
a45f217
Changed exception message
alvarosg Dec 14, 2016
3bd901e
Fixed typo
alvarosg Dec 14, 2016
c143e75
Implemented feedback from @QuLogic
alvarosg Dec 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Corrections from @efiring
  • Loading branch information
@alvarosg
alvarosg committed Dec 13, 2016
commit c5770fdcc7b1b95eb15012dd393e85cfbb5f932d
72 changes: 37 additions & 35 deletions lib/matplotlib/cbook.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2668,7 +2668,7 @@ class _FuncInfo(object):
Class used to store a function

Each object has:
Copy link
Member
@QuLogic QuLogic Dec 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This should probably be done in numpydoc style.

Class used to store a function

Attributes
----------
function : function??
    The direct function
inverse : function??
    The inverse function
bounded_0_1 : bool
    ...
check_params : callable
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is a hidden helper class for pure internal use. Most classes in these cases do not even have documentation, so I figured rules for formatting would not be as strict, so did not really put any attention to follow numpydoc... I will change it.

* The direct function (direct)
* The direct function (function)
* The inverse function (inverse)
* A boolean indicating whether the function
Copy link
Member

Choose a reason for hiding this comment

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

And why is this wrapped so early? It looks weird when the next bullet isn't.

is bounded in the interval 0-1 (bounded_0_1), or
Expand All @@ -2678,23 +2678,23 @@ class _FuncInfo(object):
certain combination of parameters is valid.

"""
def __init__(self, direct, inverse, bounded_0_1=True, check_params=None):
self.direct = direct
def __init__(self, function, inverse, bounded_0_1=True, check_params=None):
self.function = function
self.inverse = inverse
Copy link
Member

Choose a reason for hiding this comment

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

Naming preference: I suggest "function" and "inverse" rather than "direct" and "inverse".


if (hasattr(bounded_0_1, '__call__')):
if callable(bounded_0_1):
self._bounded_0_1 = bounded_0_1
else:
self._bounded_0_1 = lambda x: bounded_0_1

if check_params is None:
self._check_params = lambda x: True
elif (hasattr(check_params, '__call__')):
elif callable(check_params):
self._check_params = check_params
else:
raise ValueError("Check params must be a callable, returning "
Copy link
Member

Choose a reason for hiding this comment

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

Who is this message for? What is check params and how would the user know if they messed this up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This message is for whoever makes instances of _FuncInfo, which given the fact that it is underscored, would most likely be a developer. For now this only happens inside _StringFuncParser, which should have everything right, and never trigger the exception, but I wanted to make it robust enough to be used independently.

check_params is a callable that takes a list of the parameters and returns if that list of parameters is allowed for the function. For example:
'log{2}(x+{0.1})' will have its parameters extracted params=[2.,0.1], and will be transformed into 'log{a}(x+{a})'. Then the check_params stored for that parametric string function will be called check_params(params), and will return True.

If on the other hand if 'log{-2}(x+{0.1})', was called, then params=[-2.,0.1], and check_params(params) will return False, as -2 is not a valid value for the base.

I will make the message a bit more clear, as well and the unofficial documentation of the class.

"a boolean with the validity of the passed "
"parameters or None.")
"parameters, or None.")

def is_bounded_0_1(self, params=None):
return self._bounded_0_1(params)
Expand Down Expand Up @@ -2767,52 +2767,56 @@ def __init__(self, str_func):
String to be parsed.

"""
try: # For python 2.7 and python 3+ compatibility
is_str = isinstance(str_func, basestring)
except NameError:
is_str = isinstance(str_func, str)

if not is_str:
if not isinstance(str_func, six.string_types):
raise ValueError("The argument passed is not a string.")
Copy link
Member

Choose a reason for hiding this comment

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

"{} must be a string".format(str_func)

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 am not sure if it is a good idea to format something that we know is not a string into a string. What about simply " 'str_func' must be a string"?

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 though about it again, and realized that actually your way it is better for cases when the error shows up to an user that does not know where the exception is being raised exactly, so, thank you for your suggestion, it now produces a message like that one :).

self._str_func = str_func
self._str_func = six.text_type(str_func)
self._key, self._params = self._get_key_params()
self._func = self.func
self._func = self._parse_func()

@property
def func(self):
def _parse_func(self):
"""
Returns the _FuncInfo object, replacing the relevant parameters if
necessary in the lambda functions.
Parses the parameters to build a new _FuncInfo object,
replacing the relevant parameters if necessary in the lambda
functions.

"""

func = self._funcs[self._key]
if self._params:
m = func.direct
direct = (lambda x, m=m: m(x, self._params))
m = func.function
function = (lambda x, m=m: m(x, self._params))

m = func.inverse
inverse = (lambda x, m=m: m(x, self._params))

is_bounded_0_1 = func.is_bounded_0_1(self._params)

func = _FuncInfo(direct, inverse,
func = _FuncInfo(function, inverse,
is_bounded_0_1)
else:
Copy link
Member

Choose a reason for hiding this comment

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

since the else is so much shorter, what about flipplng:

if not self._params:
  func = _FuncInfo(func.function, func.inverse,
                            func.is_bounded_0_1())
else:
    #longer block of code

func = _FuncInfo(func.direct, func.inverse,
func = _FuncInfo(func.function, func.inverse,
func.is_bounded_0_1())
return func

@property
def directfunc(self):
def func_info(self):
"""
Returns the _FuncInfo object.

"""
return self._func

@property
def function(self):
"""
Returns the callable for the direct function.

"""
return self._func.direct
return self._func.function

@property
def invfunc(self):
def inverse(self):
"""
Returns the callable for the inverse function.

Expand All @@ -2829,30 +2833,28 @@ def is_bounded_0_1(self):
return self._func.is_bounded_0_1()

def _get_key_params(self):
str_func = six.text_type(self._str_func)
str_func = self._str_func
# Checking if it comes with parameters
regex = '\{(.*?)\}'
params = re.findall(regex, str_func)

if params:
F438 Copy link
Member

Choose a reason for hiding this comment

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

you can drop the if params' 'cause when params is none, the loop just won't happen

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 agree for the loop but there is also a piece of code after the loop and within the if block 'str_func = re.sub(regex, '{p}', str_func)', however I still think you are right, because if the original call to 'params = re.findall(regex, str_func)' returns no elements, then re.sub used with the same regex should not replace any elements.

for i in range(len(params)):
for i, param in enumerate(params):
try:
params[i] = float(params[i])
except:
raise ValueError("Error with parameter number %i: '%s'. "
"'p' in parametric function strings must "
" be replaced by a number that is not "
"zero, e.g. 'log10(x+{0.1})'." %
(i, params[i]))
params[i] = float(param)
except ValueError:
raise ValueError("Parameter %i is '%s', which is "
"not a number." %
(i, param))

str_func = re.sub(regex, '{p}', str_func)

try:
func = self._funcs[str_func]
except:
except ValueError, KeyError:
raise ValueError("%s: invalid string. The only strings "
Copy link
Member

Choose a reason for hiding this comment

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

"%s is an invalid string.

"recognized as functions are %s." %
(str_func, self._funcs.keys()))
(str_func, list(self._funcs)))

# Checking that the parameters are valid
if not func.check_params(params):
Expand Down
12 changes: 6 additions & 6 deletions lib/matplotlib/tests/test_cbook.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,22 +553,22 @@ class TestFuncParser(object):
ids=validstrings)
def test_values(self, string, func):
func_parser = cbook._StringFuncParser(string)
f = func_parser.directfunc
f = func_parser.function
assert_array_almost_equal(f(self.x_test), func(self.x_test))

@pytest.mark.parametrize("string", validstrings, ids=validstrings)
def test_inverse(self, string):
func_parser = cbook._StringFuncParser(string)
f = func_parser.func
fdir = f.direct
f = func_parser.func_info
fdir = f.function
finv = f.inverse
assert_array_almost_equal(finv(fdir(self.x_test)), self.x_test)

@pytest.mark.parametrize("string", validstrings, ids=validstrings)
def test_get_invfunc(self, string):
def test_get_inverse(self, string):
func_parser = cbook._StringFuncParser(string)
finv1 = func_parser.invfunc
finv2 = func_parser.func.inverse
finv1 = func_parser.inverse
finv2 = func_parser.func_info.inverse
assert_array_almost_equal(finv1(self.x_test), finv2(self.x_test))

@pytest.mark.parametrize("string, bounded",
Expand Down
0