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

Add dask array html repr to cube html repr #3383

Closed
wants to merge 3 commits into from

Conversation

DPeterK
Copy link
Member

@DPeterK DPeterK commented Aug 28, 2019

Extend the cube html repr to include the html repr of the cube's lazy dask array, if present. If the cube is not lazy or the dask version does not support html repr of arrays then the dask array repr is skipped without erroring.

Here's an example:
Screenshot 2019-08-28 at 16 29 51

@pp-mo
Copy link
Member

pp-mo commented Aug 28, 2019

It is not functioning for me, when I test with :

  • firefox
  • IPython 4.2.0
  • jupyter core 4.4 + client 5.3
  • dask 1.2.2

It seems that the dask arrays do not provide _repr_html_.
What do we need to make this work ?

@DPeterK
Copy link
Member Author

DPeterK commented Aug 29, 2019

What do we need to make this work ?

Dask 2.0 😉 (see the dask whatsnew)

@pp-mo when you say not working, do you mean that the entire cube repr fails (it shouldn't do that) or just that the dask array repr doesn't appear? The second is intentional, as per the description of this PR:

If ... the dask version does not support html repr of arrays then the dask array repr is skipped

Does that help?

@DPeterK
Copy link
Member Author

DPeterK commented Aug 29, 2019

BTW ping @mrocklin - I've been looking forward to combining the Iris cube html repr with the dask array html repr since the dask repr was introduced!

@pp-mo
Copy link
Member

pp-mo commented Aug 29, 2019

when you say not working, do you mean that the entire cube repr fails (it shouldn't do that) or just that the dask array repr doesn't appear?

No, it's just missing the dask repr part. I was using Python 2.7 <<slaps own wrist>> for certain convenience reasons. No Dask 2 in python 2 !

@lbdreyer lbdreyer added this to the v3.0.0 milestone Aug 29, 2019
@pp-mo
Copy link
Member

pp-mo commented Aug 29, 2019

the entire cube repr fails (it shouldn't do that)

Even so, my simple example frankly looks a bit horrible, and I'm not even clear whether it is "correct".

iris.tests.stock.istk.global_pp() 

notebook_screenshot

Obviously that might belong on another ticket

@DPeterK
Copy link
Member Author

DPeterK commented Aug 29, 2019

Ooh no that doesn't look right. In fact it looks like there's a problem with the cube's summary string that means it's not being parsed correctly. I don't think that's related at all to the changes here; you'll probably find the same behaviour on upstream Iris...

Edit: in fact, I can't reproduce the behaviour you're seeing:
Screenshot 2019-08-29 at 11 40 22

@DPeterK
Copy link
Member Author

DPeterK commented Aug 29, 2019

@lbdreyer why v3 and not v2.3? These are only small changes...

@lbdreyer
Copy link
Member

@lbdreyer why v3 and not v2.3? These are only small changes...

Admittedly I did that before properly looking at this PR. My thinking was along the lines of 'if it's not going to work in Python 2 then it could instead go into the Iris 3 release when we drop Python 2 support'. If it doesn't break under Python 2 then it can certainly go in before Iris 3.

We are under a little pressure with outstanding functionality/fixes that need to go in for Iris 2.3, so I can't guarantee this change will also get in in time, but I will take off the Iris 3 milestone/project.

@lbdreyer lbdreyer removed this from the v3.0.0 milestone Aug 29, 2019
@DPeterK
Copy link
Member Author

DPeterK commented Aug 29, 2019

Thanks @lbdreyer!

@stephenworsley
Copy link
Contributor

Just to put down some thoughts on the bug @pp-mo is seeing here, I had a look at the code and i think this is what is happening:

I believe somehow the data associated with the dimension coordinate also contains the information for scalar coordinates. At this line we begin treating this information as if we were expecting only '+' and '-' separated by whitespace. The body now consists of a list of all the 'words' and the colspan is set to 0. When making a row, we end up here, and we append each item in the list of words in the body as its own column. I'm still not sure what is causing this behaviour to occur, but I don't believe this would be caused by the changes made here.

@bjlittle bjlittle self-assigned this Oct 2, 2019
@bjlittle bjlittle added this to the v3.0.0 milestone Oct 3, 2019
@bjlittle bjlittle modified the milestones: v3.0.0, v3.1.0 Aug 10, 2020
@pp-mo
Copy link
Member

pp-mo commented Jul 30, 2021

Hi @DPeterK
I'm just considering this alongside #3376 and #3380 for inclusion in Iris 3.1.0

But I'm a bit doubtful now as to whether you/we can find time to fix these up in the next ~2-3weeks ?
So I have just for now moved all 3 back to the 'Backlog' column of the 3.1.0 project.

I'm considering these 3 together, as they all concern the Cube._repr_html().
It's possible that #4206 may break any/all of these (thought it seems valid for now?)
But in general anyway, we would now like to re-write the support based on #3987, which should make for mush easier future maintenance.

So , could you maybe update how you feel about this ?
If you're keen, I'm pretty sure that #3376 and #3380 could in fact still go in, with probably just a little work from us.
However this PR, at least, does still seem to need some more work.

@bjlittle bjlittle modified the milestones: v3.1.0, v3.2.0 Aug 4, 2021
@jamesp jamesp modified the milestones: v3.2.0, v3.3.0 Oct 28, 2021
@trexfeathers
Copy link
Contributor

This seems to have gone stale. We still have ambitions for better HTML, particularly given @pp-mo's work turning the Cube summary into actual objects, which would make for much better HTML creation. But that would need a different PR, so I'm going to close this one. @DPeterK feel free to re-open if you disagree.

Specifically on this one: we LOVE how it looks, so definitely something to revisit in the new Cube summary world.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants