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.

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
Merged

Conversation

kwmsmith
Copy link
Member

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?

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

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


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

Choose a reason for hiding this comment

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

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

@kwmsmith kwmsmith mentioned this pull request Mar 19, 2016
if use_new_repr:
return new_repr(self)
else:
# DeprecationWarnings are ignored by default, so we use a UserWarning here
Copy link
Member

Choose a reason for hiding this comment

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

we are actually using a deprecation warning here

@llllllllll
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member Author

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

@llllllllll
Copy link
Member

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
Copy link
Member Author

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
@kwmsmith kwmsmith deleted the no-compute-in-repr branch March 25, 2016 19:11
@llllllllll
Copy link
Member

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants