Skip to content

Validate that float_format option is callable #12711

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
Closed
wants to merge 1 commit into from
Closed

Validate that float_format option is callable #12711

wants to merge 1 commit into from

Conversation

tdhopper
Copy link
Contributor

@tdhopper
Copy link
Contributor Author

Some of my travis builds are failing for reasons unclear to me. This is my first attempt at a pandas PR, so I'd love some help if it's available.

Also, I'm having a hard time seeing where a test would go for this. I haven't been able to find where option validators are tested.

@jreback
Copy link
Contributor

jreback commented Mar 24, 2016

can you add a test in test_config.py (test_validation) section. and pls add a whatsnew note (bug fix is good)

@jreback
Copy link
Contributor

jreback commented Mar 24, 2016

http://pandas.pydata.org/pandas-docs/stable/contributing.html

try:

git checkout yourbranch
git rebase -i originmaster

fix anything if needed

git push yourorigin yourbranchname --tags
git push yourorigin yourbranchname

I suspect you have a fairly old fork, which means its tags aren't updating. this is an annoying git/travis thing.

@jreback jreback added the Error Reporting Incorrect or improved errors from pandas label Mar 24, 2016
@jreback jreback changed the title Validate that float_format option is callable Validate that float_format option is callable Mar 24, 2016
@tdhopper tdhopper force-pushed the validate-float_format branch 4 times, most recently from f4f5676 to 6495011 Compare March 24, 2016 19:06
@gfyoung
Copy link
Member

gfyoung commented Mar 24, 2016

Just an FYI, as I'm not sure what changes you are adding: every time you push, you start a new build on Travis, and right now you have three builds that haven't been run yet. It seems prudent perhaps to test locally before you push. Also, from a selfish standpoint, I am impatient and would like Travis to run the most recent build for my PR 😄

@jreback: Could you cancel the old builds for this PR?

@tdhopper
Copy link
Contributor Author

@gfyoung: Oy, I wasn't thinking about it triggering builds on the Pandas Travis. (I'm watching it on mine.) I can't have the tests cooking my work computer at the moment, so I was letting Travis handle it for me.

@jreback
Copy link
Contributor

jreback commented Mar 24, 2016

@gfyoung That's just how things work. I don't see it as a problem.

@jreback
Copy link
Contributor

jreback commented Mar 24, 2016

@gfyoung you of course have travis running on YOUR own branches, the pydata/pandas builder is just for final confirmation.

@tdhopper
Copy link
Contributor Author

In any case, I think I'm getting close to having this thing ready, @jreback

@jreback
Copy link
Contributor

jreback commented Mar 24, 2016

https://travis-ci.org/tdhopper/pandas

https://travis-ci.org/gfyoung/pandas (@gfyoung), not running on your branch for some reason

jreback
jreback reviewed Mar 24, 2016
View reviewed changes
doc/source/whatsnew/v0.18.1.txt
@@ -124,6 +124,7 @@ Bug Fixes
- Bug in numpy compatibility of ``np.round()`` on a ``Series`` (:issue:`12600`)
- Bug in ``Series`` construction with ``Categorical`` and ``dtype='category'`` is specified (:issue:`12574`)
- Bugs in concatenation with a coercable dtype was too aggressive. (:issue:`12411`, :issue:`12045`, :issue:`11594`, :issue:`10571`)
- Bug in `float_format` option with option not being validated as callable. (:issue:`12706`, :issue:`12711`)
Copy link
Contributor

Choose a reason for hiding this comment

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

double back-ticks on float_format, only list the issue option, not the PR one (e.g. 12706)

@jreback
Copy link
Contributor

jreback commented Mar 24, 2016

@tdhopper yep that looks good. minor doc change. ping when green.

@jreback jreback added this to the 0.18.1 milestone Mar 24, 2016
@gfyoung
Copy link
Member

gfyoung commented Mar 24, 2016

@tdhopper : Ah, no worries. Just wanted to point that out for future reference. It also was driven partially be selfish motives as I didn't try hard to conceal. 😄

@jreback : Fair point. For future reference, would that be considered valid confirmation in case there is a traffic jam on the main pandas account?

@jreback
Copy link
Contributor

jreback commented Mar 24, 2016

@gfyoung no, but its simple better to ping me if you want me to look. But I would certainly fix up your personal branch on travis, it runs MUCH faster as there is NO contention, except your own builds. pydata builds are contending with LOTS of things.

tdhopper
tdhopper reviewed Mar 24, 2016
View reviewed changes
pandas/core/config.py
@@ -803,3 +803,4 @@ def inner(x):
is_str = is_type_factory(str)
is_unicode = is_type_factory(compat.text_type)
is_text = is_instance_factory((str, bytes))
is_callable = 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.

@jreback: This doesn't work because callable doesn't raise a ValueError. I could overcome this by:

def is_callable(obj):
    """

    Parameters
    ----------
    `obj` - the object to be checked

    Returns
    -------
    validator - returns True if object is callable, and
        raises ValueError otherwise.

    """
    if not callable(obj):
        raise ValueError("Value must be an callable")
    return True

Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

yep that would work

@tdhopper tdhopper force-pushed the validate-float_format branch 2 times, most recently from 355b16c to 74c17fa Compare March 25, 2016 18:38
@tdhopper
Copy link
Contributor Author

@jreback: Sadly validator=is_instance_factory([type(None), is_callable) won't work because isinstance(x, is_callable) doesn't work.

My current solution (tested fine locally and now running in travis) is adding a is_callable_or_none validator. The only other option I can think of is a factor function that can essentially or a list of validators and pass it is_callable and is_None. However, or ends up being a pain since we're using validators that either return True and otherwise raise.

This is tricker than I thought!

@tdhopper tdhopper force-pushed the validate-float_format branch from 74c17fa to a974d66 Compare March 25, 2016 19:03
@jreback
Copy link
Contributor

jreback commented Mar 25, 2016

I think this will work.

 from types import FunctionType

In [2]: f = is_instance_factory([type(None), FunctionType])

In [3]: f(None)

In [4]: f(lambda x: x)

# you can't make this any more generic that a single function
In [5]: f(lambda x, y: x)

In [6]: f(1)
ValueError: Value must be an instance of <type 'NoneType'>|<type 'function'>

@tdhopper
Copy link
Contributor Author

@jreback: That would exclude callable objects. Maybe it's not a concern, but 😬

@jreback
Copy link
Contributor

jreback commented Mar 25, 2016

yeah I suppose. ok, then is_callable_or_none it is.

pls rebase as well.

@tdhopper tdhopper force-pushed the validate-float_format branch from a974d66 to 76d0116 Compare March 25, 2016 20:46
@tdhopper
BUG: Validate float_format setting as callable or None
086a824 
The `float_format` setting takes either None or a callable object
that returns a formatted string given a float value. Currently, the
setting isn't validated, so cryptic error messages are returned
if, for example, a format non-callable format string is given (see
\pandas-dev#12704). Add a standard validation method and use it to validate
the `float_format` setting.
@tdhopper tdhopper force-pushed the validate-float_format branch from 76d0116 to 086a824 Compare March 25, 2016 20:48
@tdhopper
Copy link
Contributor Author

Rebased and fixed a comment. I'm heading out for the weekend. Thanks for the help Jeff.

@jreback jreback closed this in 5870731 Mar 26, 2016
@jreback
Copy link
Contributor

jreback commented Mar 26, 2016

@tdhopper thanks!

I changed the approach to use is_one_of_factory....but otherwise the same

@tdhopper
Copy link
Contributor Author

Great solution! Much better that way.

@tdhopper tdhopper deleted the validate-float_format branch March 28, 2016 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Milestone
0.18.1
Development

Successfully merging this pull request may close these issues.

ERR: validation options that accept callables
3 participants
@tdhopper @jreback @gfyoung