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

[Dev] Enable title in tiles #232

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

[Dev] Enable title in tiles #232

wants to merge 3 commits into from

Conversation

nadijagraca
Copy link
Contributor

Description

Enabling title in tiles

Screenshot

Screenshot 2023-12-20 at 14 36 43

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.


graph_div = html.Div(
children=[
html.P(self._title, className="tile-title"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess html.H3 or whatever level it's at might be better than html.P here?

@@ -70,33 +71,49 @@ def __getitem__(self, arg_name: str):
return self.type
return self.figure[arg_name]

@_log_call
def pre_build(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it easy to do this in a validator for title rather than pre_build? If so then I think we should prefer that.

Comment on lines +77 to +78
self._title = self.figure._CapturedCallable__bound_arguments["title"]
self.figure._CapturedCallable__bound_arguments["title"] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self._title = self.figure._CapturedCallable__bound_arguments["title"]
self.figure._CapturedCallable__bound_arguments["title"] = None
self._title = self.figure["title"]

You should never need to access a property that has its name mangled like that. In this case we actually have a public method which does the same thanks to CapturedCallable.__getitem__.

Best not to modify the CapturedCallable in any way here if it's possible to avoid also. What I'd suggest instead is that you modify this bit of code in __call__:

        # Remove top margin if title is provided
        if fig.layout.title.text is None:
            fig.update_layout(margin_t=24)

to something like this:

        fig.layout.title.text = None
        fig.update_layout(margin_t=24)

Note one limitation of doing this is that users will no longer be able to edit the figure title through a parameter, but I'm ok with that.

I just thought through various different ways to handle this (e.g. should we take the title from CapturedCallable["title"] or from fig.layout.title.text?). Each has various pros and cons and none of them are perfect. The solution I propose here sounds best to me but please say if you think I forgot a better solution @maxschulz-COL since I remember discussing this with you before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Very interesting. Just to jog my memory a bit, did we decide on implementing the tiles as presented here, ie as part of the Graph model?

I also remember that we discussed the title bit, but I cannot recall if we came to a conclusion.

As for the two cases:

  • take the title from CapturedCallable["title"]: this has the fundamental problem of making this functionality argument dependent. What if I create a custom chart and call my title argument scatter_title. In this case this would not work right?
  • take it from fig.layout.title.text: This seems to me the more robust approach, as no matter how I call my arguments, the title of the layout should always be here, so this is my preferred approach at first thought

Now for the question of not updating titles through parameters, I am undecided. On the one hand I think this is a functionality that may be hardly if ever used, on the other hand, I think this introduces a dangerous pattern of not allowing parameters to target any argument.

I think we previously discussed also a slightly less automated version:

  • allow the graph model a title parameter that is public
  • do not delete the chart title, but have the user remove it manually (set to None) if they do not want two titles
  • this has other downsides, but thought I'd mention it

Copy link
Contributor

Choose a reason for hiding this comment

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

Very interesting. Just to jog my memory a bit, did we decide on implementing the tiles as presented here, ie as part of the Graph model?

Yes, exactly 👍

As for the two cases:

  • take the title from CapturedCallable["title"]: this has the fundamental problem of making this functionality argument dependent. What if I create a custom chart and call my title argument scatter_title. In this case this would not work right?

Yes, that would not work. For me this is ok so long as all the plotly.express graph have the argument called title?

  • take it from fig.layout.title.text: This seems to me the more robust approach, as no matter how I call my arguments, the title of the layout should always be here, so this is my preferred approach at first thought

Agree this is a more robust approach because it also handles the case that you do not specify the title at all through an argument but still have a graph title, like this:

@capture("graph"):
def custom_chart():
    return px.scatter(..., title="blah")

Here if you tried to extract title by fig["title"] it wouldn't give you anything, but fig.layout.title.text would.

The big problem with approach is that in order to get fig.layout.title.text, you have to actually do the __call__ to generate the figure first. This is a relatively expensive operation (e.g. if you have a lot of data), and you're basically making an entire figure just to find the title. Then you throw away the whole graph and have to rebuild the whole thing anyway. tbh this is ok but just feels kind of wasteful when in 99% of cases I guess that the title of the graph has been specified through the argument directly.

So it's doing something which is quick and easy and works in I guess 99% of cases vs. something which is wasteful but works in 100% of cases. I guess we could have some hybrid approach where first we try to do fig["title"], and only if that doesn't exist do the actual __call__ and look in fig.layout.title.text. But probably that's just overcomplicating it it's just an edge case.

Now for the question of not updating titles through parameters, I am undecided. On the one hand I think this is a functionality that may be hardly if ever used, on the other hand, I think this introduces a dangerous pattern of not allowing parameters to target any argument.

I also don't much like it as a constraint, but I think the alternative will just be more complicated than is worth it. There are workarounds if people really want to do this (write an action/callback), but it wouldn't work as a pure Parameter.

This problem is equal for both approaches btw (using fig["title"] and fig.layout.title.text).

I think we previously discussed also a slightly less automated version:

  • allow the graph model a title parameter that is public
  • do not delete the chart title, but have the user remove it manually (set to None) if they do not want two titles
  • this has other downsides, but thought I'd mention it

This is also ok by me. It's certainly an easy solution and would work well (apart from the Parameter thing which is still not possible). Just it feels like in 99% of cases we can easily solve this for the user already without them having to move the title value from their plotting function to Graph.title. This is why I think I'd favour the "quick and easy look up fig["title"]" which makes people's lives easier and works most of the time, even though it's not perfect. We could also make Graph.title public (probably good idea anyway for consistency with Table) and take precedence over fig["title"] when it is set.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good points! And very important point on the performance.

I don't understand one thing though

This is also ok by me. It's certainly an easy solution and would work well (apart from the Parameter thing which is still not possible).
Why is that? if we do not delete the chart title, but leave everything as is, this should behave like before?

All things considered I feel we may want to start with the third approach, and then we can still go either way if it is actually important. Maybe we end up liking it enough such that we do not need to force the automation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes sorry, you're right that the 3rd approach does indeed allow changing the title using a Parameter so long as you specify it through the plotting function and not Graph.title. That is a definite advantage of that approach.

The biggest disadvantage I guess is that it looks a bit weird/confusing to have a title field in two places (Graph and the plotting function), and that they render a bit differently on the screen.

I'm happy to go with this as the solution anyway and see if anyone finds it awkward or if it's actually fine in reality. It's definitely the easiest solution.

@huong-li-nguyen huong-li-nguyen changed the title POC - enabling title in tiles [Dev] Enable title in tiles Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants