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

Improve frames support in graph_objs.py. #656

Merged
merged 4 commits into from
Jan 11, 2017
Merged

Conversation

theengineear
Copy link
Contributor

@theengineear theengineear commented Jan 9, 2017

This does the following:

  • Allow object-or-string in frames array
  • Refuse frames in frame object if nested
  • Map frame entry data and layout objects to figure attribute information.
  • Tests this functionality

Fixes #604

@theengineear
Copy link
Contributor Author

@Kully and I hacked in frames support when we were in a rush. This improves how frames is integrated into the Figure object. Since frames is basically considered an array of Figure objects.

Still working on this one, since I think saying that an entry in a Frames array is a Figure is misleading, imo...

@theengineear
Copy link
Contributor Author

Can't add Frame since it conflicts with animation.frame and the name doesn't matter since we're not exposing it :). I chose frame_entry, which does not become a public class because there's no need to patch the object. Note how it ends up as dict( .. ) in the to_string() call.


def _value_to_graph_object(self, index, value, _raise=True):
if isinstance(value, six.string_types):
return value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a patch to allow a string in the frames array.

indent=' ' * indent * (level + 1))
for index, entry in enumerate(self):
if isinstance(entry, six.string_types):
string += repr(entry)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another patch to allow a string in the frames array. This sorta sucks since it's not possible to super... but I'm not terribly concerned.

else:
# There are no lists objects which can contain list entries.
items_classes.add('dict')
items_classes = list(items_classes)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows us to print dict in a list's to_string() return if the item isn't a publicly-available class.

'group': {'role': 'info'},
'layout': {'role': 'object'},
'name': {'role': 'info'}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard or @rreusser Is this information available anywhere? I don't really want to hard-code these. If it's not available, mind helping me to flesh out this list of valid attributes for a figure.frames[*] entry?

Copy link

@rreusser rreusser Jan 9, 2017

Choose a reason for hiding this comment

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

Unfortunately there's not a dedicated doc page since this doesn't currently live on the plot scheme. In lieu of that, the attributes are defined here. @etpinard is there some way I can add these to the plot schema? The way animation config and frame attributes live outside of the rest of plotly is not the best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome! That'll do for now!

Copy link
Contributor

Choose a reason for hiding this comment

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

Those are now part of the plot schema:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh damn boom!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard , trying to grok datasrc, that one's a little confusing for me. How do you src an array of objects?

Also, traces are meant to be stored in a Grid? I.e., it's definitely not info, right? (just to confirm).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh datasrc shouldn't be in there. Same for tracessrc.

@theengineear
Copy link
Contributor Author

@etpinard , OK, this would work well from the Python side of things for the frames object:

    "frames": {
        "items": {
            "frames_entry": {
                "baseframe": {
                    "description": "The name of the frame into which this frame's properties are merged before applying. This is used to unify properties and avoid needing to specify the same values for the same properties in multiple frames.",
                    "role": "info",
                    "valType": "string"
                },
                "data": {
                    "description": "A list of traces this frame modifies. The format is identical to the normal trace definition.",
                    "role": "object",
                    "valType": "any"
                },
                "group": {
                    "description": "An identifier that specifies the group to which the frame belongs, used by animate to select a subset of frames.",
                    "role": "info",
                    "valType": "string"
                },
                "layout": {
                    "role": "object",
                    "description": "Layout properties which this frame modifies. The format is identical to the normal layout definition.",
                    "valType": "any"
                },
                "name": {
                    "description": "A label by which to identify the frame",
                    "role": "info",
                    "valType": "string"
                },
                "traces": {
                    "description": "A list of trace indices that identify the respective traces in the data attribute",
                    "role": "info",
                    "valType": "info_array"
                },
                "role": "object"
            }
        },
        "role": "object"
    },

The name frames_entry is up for discussion but it would greatly help me if it's unique in the plot-schema, fwiw... i.e., not frame. This is because it's tricky to map data and layout attrs properly inside frames_entry objects.

Note that FrameEntry will not be a name that is exposed to users in the Python API, it's merely helpful for mapping attrs.

@theengineear
Copy link
Contributor Author

OK, I "updated" the default schema with what I'd like to be in there. Obviously not ready to merge since this isn't a real reflection of the plot-schema, I just figured It'd be helpful.

@theengineear
Copy link
Contributor Author

Oh right, we have a test on the schema anyhow so I can't merge it 😊

@etpinard
Copy link
Contributor

@theengineear done (I think) in plotly/plotly.js#1292

This does the following:

* Allow object-or-string in frames array
* Refuse `frames` in frame object if nested
* Generates a non-public `FramesEntry` graph obj
* Tests this functionality
graph_reference = utils.decode_unicode(_json.loads(s))

# TODO: Patch in frames info until it hits streambed. See #659
graph_reference['frames'] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just patching in what I'm expecting to see here. This means that we don't need to wait for any changes to land on Plotly prod and can get this into 1.X now. I'll remove this once that changes.

@theengineear
Copy link
Contributor Author

@Kully, this look OK to you? Mind giving it a review?

@Kully
Copy link
Contributor

Kully commented Jan 11, 2017

@Kully, this look OK to you? Mind giving it a review?

Yeah, will do today.

@Kully
Copy link
Contributor

Kully commented Jan 11, 2017

Yeah, will do today.

@theengineear 💃 Looks like a lot of work you had to do! 😅

@theengineear
Copy link
Contributor Author

Yah, the fact that you can have data and layout inside the frame was tricky. We could have just fixed this in plot-schema, but that would have taken a whole lot more work I think.

@theengineear
Copy link
Contributor Author

Cool, I'm able to play around and make frames and such using the python api lib still. Gonna update changelog and merge.

@theengineear theengineear merged commit fa89dcc into master Jan 11, 2017
@theengineear theengineear deleted the frames-support branch January 11, 2017 22:55
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

4 participants