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

Gantt chart to scatterplots #1665

Merged
merged 21 commits into from
Aug 3, 2019
Merged

Gantt chart to scatterplots #1665

merged 21 commits into from
Aug 3, 2019

Conversation

cs48a
Copy link
Contributor

@cs48a cs48a commented Jul 14, 2019

Gantt chart now uses scatter plots:

  • updating all the functions accordingly
  • unit tests updated
  • new param: show_hover_fill for hiding the text for the filled area

I updated the Gantt chart to use scatter plots with "toself" fills to draw the recantgles, instead of shapes. This allows hiding and isolating traces.
I tested on all the examples and the results were visually the same as with the shapes.

cs48a and others added 5 commits July 15, 2019 00:29
Gantt chart now uses scatter plots:
- updating all the functions accordingly
- unit tests updated 
- new param: show_hover_fill for hiding the text for the filled area
@jonmmease
Copy link
Contributor

Hi @csabaszan, this sounds like a great update. Thanks for taking it on. Don't worry about the *-plot_ly tests, these have been fixed in the v4_integration branch that will be merged into master shortly. It looks like there is something failing in the *-optional tests related to this PR, though.

@cs48a
Copy link
Contributor Author

cs48a commented Jul 16, 2019

Yeah, first ever pull request... but I'm on it! Thanks for the feedback

@nicolaskruchten
Copy link
Contributor

Thanks for the PR!

Can you tell us a bit more about the motivation for this change? I'd love some more insight into what is causing a problem with the existing figure factory that can't be solved by remaining within the bar paradigm? :)

@nicolaskruchten
Copy link
Contributor

Sorry, I just looked into it a bit more and the current implementation isn't using bar, I just inferred from your description above :)

@cs48a
Copy link
Contributor Author

cs48a commented Jul 16, 2019

@nicolaskruchten I was trying to toogle the traces and when they failed I found #1371 where @jonmmease mentioned that fill "toself" could be a way to go.

Now except for the *-plot_ly tests the tests are successful

@jonmmease
Copy link
Contributor

Hi @csabaszan,

Now that we merged the v4_integration branch and release version 4, we'll need to update this branch to fix the merge conflicts.

Basically, all of the files you modified have moved under the packages/python/plotly directory. I don't think there were any changes in these files other than the move, so you should be able to overwrite the current files in the new locations with your latest versions.

If you run into trouble doing this as a merge resolution with git, feel free to start over and create a new pull request. Thanks!

@nicolaskruchten
Copy link
Contributor

Having looked at this a bit more closely, I would actually recommend using bar traces rather than making rectangles with scatter traces, unless there's a compelling reason not to @jonmmease ?

michaelbabyn and others added 12 commits July 21, 2019 22:30
* only set plotlyServerURL using the plotly_domain in the .config file
if one isn't specified in the config dict passed to iplot/plot
* make it clear that the default in clean_config.get is None
* scraper function for sphinx-gallery, in plotly.io.  scraper function + skeleton of sphinx doc with minimal gallery (two examples)

* added psutil dep to tox.ini

* python 2 compatibility

* added orca dep to package.json for tests

* removed typo

* corrected bug

* python 2 compatibility

* added electron

* try to configure orca

* Moved test file so that it's tested by the orca build only.

* use custom renderer for sphinx-gallery

* removed sphinx files which are now in plotly-sphinx-gallery

+ changed name of renderer sphinx -> sphinx-gallery, and changed wording
of documentation

* Modified test with new renderer name

* try to fix py2 compatibility

* write_html instead of offline.plot

* change import order
Gantt chart now uses scatter plots:
- updating all the functions accordingly
- unit tests updated 
- new param: show_hover_fill for hiding the text for the filled area
# Conflicts:
#	packages/python/plotly/plotly/tests/test_optional/test_figure_factory/test_figure_factory.py
#	packages/python/plotly/tox.ini
# Conflicts:
#	packages/python/plotly/plotly/figure_factory/_gantt.py
#	packages/python/plotly/plotly/tests/test_optional/test_figure_factory/test_figure_factory.py
#	packages/python/plotly/plotly/tests/test_optional/test_tools/test_figure_factory.py
@jonmmease
Copy link
Contributor

Having looked at this a bit more closely, I would actually recommend using bar traces rather than making rectangles with scatter traces, unless there's a compelling reason not to @jonmmease ?

I honestly don't remember if there was a good reason why I mentioned scatter+fill in #1371 rather than bar. And you're right that bar would be more natural. The one factor that comes to mind is that both the shape and scatter+fill methods give full control over the width of the bars in data coordinates, but it's not clear to me that this would really be needed.

@csabaszan, do you see any reason to use scatter+fill rather than bar? For the bar approach, we would use bars in horizontal mode (https://plot.ly/python/horizontal-bar-charts/#basic-horizontal-bar-chart), specifying a custom base for each bar (https://plot.ly/python/bar-charts/#customizing-individual-bar-base).

@cs48a
Copy link
Contributor Author

cs48a commented Jul 22, 2019

@csabaszan, do you see any reason to use scatter+fill rather than bar? For the bar approach, we would use bars in horizontal mode (https://plot.ly/python/horizontal-bar-charts/#basic-horizontal-bar-chart), specifying a custom base for each bar (https://plot.ly/python/bar-charts/#customizing-individual-bar-base).

@jonmmease I've just looked into bar and yes, I think with horizontal mode and using the base it can be done. I'll try do the switch and update the PR.

image

@cs48a
Copy link
Contributor Author

cs48a commented Jul 22, 2019

btw what would be the best way to handle timedelta in the x-axis? I can set the type of the axis to date, but I don't know what should x take as an input. I was trying to use it with seconds and milliseconds but it was somewhat strange, because 23-hours is displayed as 1 day.

The code below produces the plot in the image. Resource 2 is shown to be occupied for 1 day, but according to the data it is only 23 hours

from plotly import graph_objs as go
fig = go.Figure()
fig.add_trace(go.Bar(
    y=["resource 1", "resource 2", "resource 3"],
    x=[3600*24*1000, 3600*23*1000, 3600*22*1000],
    base=["2016-01-01 6:00:00", "2016-01-01 6:00:00", "2016-01-01 6:00:00"],
    name='Job1',
    orientation='h',
    text=["a", "b", "c"],
))
fig.layout.barmode='overlay'
fig.layout.xaxis.type = "date"

image

What really interesting, though, is that if I set x to 0 then a 1-hour long bar is displayed, so could it be that somehow this 1 hour offset is always there?

@jonmmease
Copy link
Contributor

Hi @csabaszan,

Yeah, that's interesting. It looks like there might be something timezone related creeping in when specifying the x value as a numeric value in milliseconds. Here's what I get for your same plot in GMT-4.

newplot (17)

In any case, plotly.js doesn't have a timedelta axis type, and I don't think it will be possible to use the date axis type to display times larger that 24 hrs.

@cs48a
Copy link
Contributor Author

cs48a commented Jul 24, 2019

Hi @jonmmease,

Do you think there is some workaround to use bar for Gantt charts? Displaying the length of the period in a readable format is pretty important for a Gantt, imo.

@jonmmease
Copy link
Contributor

Hi @csabaszan,

Since the current API is to specify the start and end of each "bar" as a date, it might be better to stick with your current scatter+fill approach. Maybe this is what I was thinking when I brought up scatter in the first place 🙂

@nicolaskruchten, do you any thoughts on what might be going on with the bar lengths above?

@jonmmease
Copy link
Contributor

jonmmease commented Jul 27, 2019

Thanks @csabaszan, this looks great! It's really nice to have the legend control. In addition to the inline comments, there's one other change that would be nice to have.

Right now, when you click the legend to hide the bars, the hover points are still visible as small dots. I made some inline comments about making these invisible by setting opacity=0, but even with this change the tooltips will still appear when the bar is hidden. You can make these hover points disappear along with the bar by placing them in the same legendgroup as the associated scatter trace (See https://plot.ly/python/legend/#grouped-legend). To do this, set the legendgroup property of both the filled scatter trace, and the hover scatter trace to be equal to the trace's name.

thanks again!

@nicolaskruchten
Copy link
Contributor

If you put multiple traces in the same legend group, it’s nice to have all but one per group not appear in the legend, that way it looks like just a single legend entry

legendgroups for markers
marker opacity is 0
@jonmmease
Copy link
Contributor

Thanks for the last round of updates @csabaszan. Looks good to me!

@jonmmease jonmmease merged commit f42d248 into plotly:master Aug 3, 2019
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

5 participants