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

Incorporate Plotly JSON to metamodel (Closes #1745) #1747

Merged
merged 24 commits into from
Jul 25, 2020
Merged

Conversation

umesh-timalsina
Copy link
Contributor

@umesh-timalsina umesh-timalsina commented Jul 8, 2020

Checklist:

  • Update pipeline and project seeds
  • Change backend_deepforge to plotly_backend for plots
  • Change ExecutionIndex and PlotlyGraph panels/ widget to incorporate multiple plotly Graphs instead of instead of combining them (Could be changed)
  • Update plotly_backend to incorporate 3DScatter and 3DLine Plots
  • Update Tests to incorporate changed pipeline seeds.
  • Add Change metamodel for plotly related Updates.

@umesh-timalsina umesh-timalsina marked this pull request as ready for review July 10, 2020 21:55
renderer = DeepforgePlotlyRenderer()
exporter = mplexporter.Exporter(renderer)
exporter.run(fig)
renderer.crawl_3d_labels(fig)
Copy link
Contributor Author

@umesh-timalsina umesh-timalsina Jul 13, 2020

Choose a reason for hiding this comment

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

renderer.resize() method removes width, height and other properties for a plotly figure, which is fine. But, this is already done in PlotyGraphWidget. Can be done here as well.

By default, PlotlyRenderer tries its hardest to precisely mimic an
        mpl figure. However, plotly is pretty good with aesthetics. By
        running PlotlyRenderer.resize(), layout parameters are deleted. This
        lets plotly choose them instead of mpl.

Copy link
Contributor

@brollb brollb left a comment

Choose a reason for hiding this comment

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

Nice work! A couple minor things then it should be good!

@@ -23,8 +25,9 @@ define([
this.$el.css('overflow', 'auto');
this.$el.addClass(WIDGET_CLASS);
this.nodes = {};
this.plotlyJSON = null;
this.plotlyJSONS = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

As a pretty minor comment, this should probably be this.plotlyJSONs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it's not clear to me that it needs to be saved on this. If possible, it would be preferable to avoid this extra statefulness and simply pass the the plotly JSON(s) to the functions that require them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this.nodes, this.layout, this.created, and this._container appear to be unused (or unnecessary).

}
this.addAxesScatterPoints(axesNode, this.node, axes);
});
this.core.setAttribute(this.node, 'data', state);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the data attribute is a string, so it would be good to JSON.stringify(state). This also makes it so we can view the contents in the default webgme UI as it cannot show arbitrary JSON.

1. Merge Branch Master
2. Apply updates to the new Metamodel
@@ -21,7 +35,7 @@ define([
});
},
apply: function(core, rootNode, META) {
// Create 'MyUtilities' node
// Create 'MyUtilities' nodeupdate
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this changed? It seems like it was probably unintentional.

const versionString = core.getAttribute(pipelineRoot, 'version');
const version = new Version(versionString);
return version.lessThan(new Version('0.22.0')) &&
version.greaterThan(new Version('0.19.1'));
Copy link
Contributor

Choose a reason for hiding this comment

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

What if it is below 0.19.1?

Copy link
Contributor Author

@umesh-timalsina umesh-timalsina Jul 23, 2020

Choose a reason for hiding this comment

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

The changes to the metamodel i.e. adding Subgraphs, Support for 3D plots and other metanodes like [Line, ScatterPoints, etc...] was introduced in pipeline seed version 0.19.1.

Version,
Q
) {
const GRAPH = 'Graph';
const getGraphNode = async function(core, rootNode, graphNodes={}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rootNode only appears to be the root node on the first call. This should probably be node instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, why are you setting the graph nodes in a dictionary rather than simply returning a list of graph nodes from the given subtree?

It would probably be good to rename the function to getGraphNodes or something so it is obvious that it returns multiple nodes.

apply: async function(core, rootNode, META) {
let graphNodes = {};
await getGraphNode(core, rootNode, graphNodes);
const graphNodeKeys = Object.keys(graphNodes);
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like you use graphNodeKeys for anything but fetching nodes from the graphNodes dictionary. A list would probably be more fitting.

@@ -2,3 +2,5 @@
{"version":"0.21.0","changelog":"Add provenance to metadata (via WithProvenance mixin)"}
{"version":"0.21.1","changelog":"Update Inheritance of Subgraph, Line, Images, ScatterPoints etc.. nodes"}


{"version":"0.22.0","changelog":"Incorporate PlotlyJSON into Graph metaNode"}
Copy link
Contributor

Choose a reason for hiding this comment

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

minor thing but this should probably be "meta node" rather than camel case.

this.deleteChart();
} else {
if (!this.created && !_.isEmpty(this.plotlyJSON)) {
Plotly.newPlot(this.$el[0], this.plotlyJSON);
if (!this.created && !_.isEmpty(plotlyJSONs)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this.created is needed, is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably also remove _.isEmpty check. Is there a reason why this should ever be the case? If not, this is just preventing it from failing fast.

@umesh-timalsina
Copy link
Contributor Author

umesh-timalsina commented Jul 21, 2020

Currently, matplotlib 3.3.0 is out. This removed method is_frame_like function(used by plotly renderer) for matplotlib.spines.Spine , which is causing any plot operation to fail.

ERROR conda.cli.main_run:execute(32): Subprocess for 'conda run ['python', 'main.py']' command failed.  (See above for error)
Traceback (most recent call last):
  File "main.py", line 31, in <module>
    result = operation.execute()
  File "/tmp/deepforge-local-exec-755318d95a65cc2ca6deb6fab5343cf9bb763c36/operations/plots_pipeline.py", line 28, in execute
    plt.show()
  File "/home/umesh/anaconda3/envs/deepforge_124/lib/python3.7/site-packages/matplotlib/pyplot.py", line 336, in show
    return _backend_mod.show(*args, **kwargs)
  File "/tmp/deepforge-local-exec-755318d95a65cc2ca6deb6fab5343cf9bb763c36/plotly_backend.py", line 638, in show
    manager.canvas.send_deepforge_update()
  File "/tmp/deepforge-local-exec-755318d95a65cc2ca6deb6fab5343cf9bb763c36/plotly_backend.py", line 694, in send_deepforge_update
    state = self.figure_to_state()
  File "/tmp/deepforge-local-exec-755318d95a65cc2ca6deb6fab5343cf9bb763c36/plotly_backend.py", line 702, in figure_to_state
    figure
  File "/tmp/deepforge-local-exec-755318d95a65cc2ca6deb6fab5343cf9bb763c36/plotly_backend.py", line 206, in mpl_to_plotly
    exporter.run(fig)
  File "/home/umesh/anaconda3/envs/deepforge_124/lib/python3.7/site-packages/plotly/matplotlylib/mplexporter/exporter.py", line 51, in run
    self.crawl_fig(fig)
  File "/home/umesh/anaconda3/envs/deepforge_124/lib/python3.7/site-packages/plotly/matplotlylib/mplexporter/exporter.py", line 118, in crawl_fig
    self.crawl_ax(ax)
  File "/home/umesh/anaconda3/envs/deepforge_124/lib/python3.7/site-packages/plotly/matplotlylib/mplexporter/exporter.py", line 123, in crawl_ax
    props=utils.get_axes_properties(ax)):
  File "/home/umesh/anaconda3/envs/deepforge_124/lib/python3.7/contextlib.py", line 112, in __enter__
    return next(self.gen)
  File "/home/umesh/anaconda3/envs/deepforge_124/lib/python3.7/site-packages/plotly/matplotlylib/mplexporter/renderers/base.py", line 57, in draw_axes
    self.open_axes(ax=ax, props=props)
  File "/tmp/deepforge-local-exec-755318d95a65cc2ca6deb6fab5343cf9bb763c36/plotly_backend.py", line 422, in open_axes
    super().open_axes(ax, props)
  File "/home/umesh/anaconda3/envs/deepforge_124/lib/python3.7/site-packages/plotly/matplotlylib/renderer.py", line 169, in open_axes
    bottom_spine = mpltools.get_spine_visible(ax, "bottom")
  File "/home/umesh/anaconda3/envs/deepforge_124/lib/python3.7/site-packages/plotly/matplotlylib/mpltools.py", line 368, in get_spine_visible
    spine_frame_like = spine.is_frame_like()
AttributeError: 'Spine' object has no attribute 'is_frame_like'

@brollb
Copy link
Contributor

brollb commented Jul 21, 2020

I would just leave updating to matplotlib 3.3.0 for another PR rather than letting this PR grow to include separate concerns.

@umesh-timalsina
Copy link
Contributor Author

Are you suggesting we pin matplotlib to version 3.2.2 for this PR?

1. Pin matplotlib to v3.2.2 in environment.worker.yml and devProject
2. Remove this.created in PlotlyGraphWidget
3. Fix releases.jsonl
4. Rename getGraphNode to getGraphNodes in updates.js, use array instead of dictionary
5. Remove extra line in ExecuteJob.spec.js
-- In version 0.21.1 of the pipeline library, Inheritance of the old
SubGraph and below was changed from FCO to Metadata (because of provenance).
So, the current Extractor to extract plotlyJSON expects this inheritance.
For older seeds, a function to add META['pipeline.Metadata'] as a Mixin is added to the
updating capabilities.

-- remove wrongfully pased comment
@umesh-timalsina
Copy link
Contributor Author

umesh-timalsina commented Jul 23, 2020

The following gist contains different plots and corresponding deepforge operations that generated them. Cuurrently, Piecharts are not supported. This should be maintained as a separate issue once #1747 is merged.

https://gist.github.com/umesh-timalsina/7a5ba1fa6b7971d632f879b2733f8c95

@brollb
Copy link
Contributor

brollb commented Jul 24, 2020

I have been testing this and ran into the following exception when switching to the ExecutionIndex view:
DeepinScreenshot_select-area_20200724083020

It might be nice to make that code a little more pessimistic. (I am not sure of the underlying cause of the issue but it is trying to parse undefined.)

@brollb
Copy link
Contributor

brollb commented Jul 24, 2020

Another minor issue. It's a little annoying that when I have only a single plot shown that it doesn't take the whole space like before:
Before:
DeepinScreenshot_xterm_20200724093638
After:
DeepinScreenshot_20200724093644

That said, I like how it looks when showing multiple plots:
DeepinScreenshot_20200724093847

@brollb
Copy link
Contributor

brollb commented Jul 24, 2020

Other than that, it looks good!

@umesh-timalsina
Copy link
Contributor Author

umesh-timalsina commented Jul 24, 2020

I have been testing this and ran into the following exception when switching to the ExecutionIndex view:
DeepinScreenshot_select-area_20200724083020

It might be nice to make that code a little more pessimistic. (I am not sure of the underlying cause of the issue but it is trying to parse undefined.)

I am not able to replicate this error. I have changed the code such that it checks if node.getAttribute('data') is truthy.
Also, I have changed the area for a single plot to take the full area of the container.

image

@brollb
Copy link
Contributor

brollb commented Jul 25, 2020

Nice work, @umesh-timalsina!

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.

2 participants