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

Deprecate compute-in-repr behavior. #1414

Merged
merged 11 commits into from Mar 25, 2016

Conversation

Projects
None yet
2 participants
@kwmsmith
Member

kwmsmith commented Feb 12, 2016

Start process to replace Expr.repr with a more standard
implementation.

Adds a peek method to do what __repr__ used to do.

@llllllllll For the life of me I can't figure out how to configure the warning filters so that the warning only shows up once -- do you have any guidance here?

Deprecate compute-in-repr behavior.
Start process to replace Expr.__repr__ with a more standard
implementation.

Adds a `peek` method to do what `__repr__` used to do.

@kwmsmith kwmsmith added this to the 0.10 milestone Feb 12, 2016

Expr.__repr__ = _warning_repr
Expr.peek = lambda x: print(expr_repr(x))
Expr._repr_html_ = lambda x: to_html(x)

This comment has been minimized.

@llllllllll

llllllllll Feb 12, 2016

Member

we should also make the _repr_html_ not implicitly compute when in jupyter.

Expr.__repr__ = _warning_repr
Expr.peek = lambda x: print(expr_repr(x))

This comment has been minimized.

@llllllllll

llllllllll Feb 12, 2016

Member

I really love this name, it totally conveys the expected use case of "peeking" at the result while working on some expression!

This comment has been minimized.

@kwmsmith

kwmsmith Feb 12, 2016

Member

Good -- I wanted something shorter than preview...

This comment has been minimized.

@llllllllll

llllllllll Feb 12, 2016

Member

One comment I have is that if we are going to call print, could we accept a file argument so that users can pass a StringIO or other output stream?

@kwmsmith kwmsmith referenced this pull request Mar 19, 2016

Closed

Blaze release 0.10 #1455

if use_new_repr:
return new_repr(self)
else:
# DeprecationWarnings are ignored by default, so we use a UserWarning here

This comment has been minimized.

@llllllllll

llllllllll Mar 24, 2016

Member

we are actually using a deprecation warning here

@llllllllll

This comment has been minimized.

Member

llllllllll commented Mar 24, 2016

Other than the comment this looks good. Regarding the visibility of the warning: I think that we should not be forcing warnings to be more visible than they are by default. If users do not want to see deprecation warnings then that is on them.

@kwmsmith kwmsmith removed the wip label Mar 25, 2016

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Mar 25, 2016

@llllllllll thanks for the feedback. I addressed your comment, updated whatsnew, and tweaked the __repr__ implementations for Symbol, _Data, and Expr to be more consistent:

In [1]: import blaze as bz

In [2]: t = bz.symbol('t', dshape='var * {name: string, id: int}')

In [3]: t
Out[3]: <`t` symbol; dshape='var * {name: string, id: int32}'>

In [4]: d = bz.data([['Alice', 1], ['Bob', 2]], dshape='var * {name: string, id: int32}', name='d')

In [5]: d
Out[5]: <'list' data; _name='d', dshape='var * {name: string, id: int32}'>

In [6]: d.name
Out[6]:
    name
0  Alice
1    Bob

In [7]: bz.interactive.use_new_repr = True

In [8]: d.name
Out[8]: <`Field` expression; dshape='var * string'>

In [9]: t.id ** 2
Out[9]: <`Pow` expression; dshape='var * int64'>

In [10]: str(t.id ** 2)
Out[10]: 't.id ** 2'

In [11]: d.name.peek()
Out[11]:
    name
0  Alice
1    Bob

Things to point out:

  • The __repr__s are all boring reprs now that show the dshape along with the type of the thing.
  • Long dshapes are truncated with a ... at the end.
  • The string form of an expression--whether compound or atomic--uses the underlying _name of the leaf terms to keep the string form looking nice.
@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Mar 25, 2016

I'm still looking into the best way to address the doctest failures.

@llllllllll

This comment has been minimized.

Member

llllllllll commented Mar 25, 2016

The doctest failures look like they are just relying on the new symbol repr, I think it is safe to just change them. I think we might want to include a bit more data in the reprs as we go, but we can do that after merging this. One immediate thing I am thinking of is including the field name in Field repr.

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Mar 25, 2016

The doctest failures look like they are just relying on the new symbol repr, I think it is safe to just change them.

+1 -- although I didn't expect you to say that :)

I think we might want to include a bit more data in the reprs as we go, but we can do that after merging this. One immediate thing I am thinking of is including the field name in Field repr.

+1.

@kwmsmith kwmsmith merged commit 5622ba4 into blaze:master Mar 25, 2016

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.1%) to 89.198%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kwmsmith kwmsmith deleted the kwmsmith:no-compute-in-repr branch Mar 25, 2016

@llllllllll

This comment has been minimized.

Member

llllllllll commented Mar 25, 2016

I'm really glad to see this feature merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment