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

[Broken Documentation] API change for figurefirst:mplmethods? #57

Open
alchem0x2A opened this issue Jun 4, 2020 · 3 comments
Open

[Broken Documentation] API change for figurefirst:mplmethods? #57

alchem0x2A opened this issue Jun 4, 2020 · 3 comments
Labels
enhancement User Documentation Documentation outside the codebase.

Comments

@alchem0x2A
Copy link

Hi

Thanks a lot for the wonderful package! I really enjoyed the workflow of FigureFirst and is gradually checking the examples.


I think the documentation for the axis methods is somehow broken (https://figurefirst.readthedocs.io/en/latest/examples.html#axis-methods), as the code in the example has no effect to draw an hspan.

From what I understand, in version 0.0.6 you need to add a node figurefirst:mplmethods inside the figurefirst:axis node. Is that correct?


Also there're some side thoughts, I propose to add another functionality FigureLayout.set_axis_props which parse the attributes within figurefirst:axis except reserved names like figurefirst:name, as a property of mpl.axes.Axes, and set the value using the setter.

For instance:

In the current version the hierarchy is:

figurefirst:axis node
   figurefirst: mplmethods node
        figurefirst:set_yticks="[]"

While in the proposed implementation
In the current version the hierarchy is:

figurefirst:axis node
    figurefirst:yticks="[]"

I may have several reasons for this:

  1. The readability is better than the mplmethods
  2. Explicitly writing the set_ prefix can be omitted, as from my personal usage, most likely the mplmethod of an axis instance is to call a setter of the Axes class, e.g. ax.set_yticks=[] for a subplot.
  3. In this case mplmethod can be some functions that solely does simple plotting / patching jobs.

I appreciate much your feedback / suggestions.Happy to contribute PRs if you find it reasonable.

@psilentp
Copy link
Member

psilentp commented Jun 5, 2020

Glad you're finding the library useful!

From what I understand, in version 0.0.6 you need to add a node figurefirst:mplmethods inside the figurefirst:axis node. Is that correct?

Yes, the documentation is incorrect great catch! We should fix this! However I believe the correct hierarchy should have the figurefirst:mplmethods node under the svg rect node as is generated by the extensions.

Screen Shot 2020-06-04 at 10 53 54 PM

Also there're some side thoughts, I propose to add another functionality FigureLayout.set_axis_props

I love this idea, particularly if we can implement it without calling eval as we do in the apply_mpl_methods function.

I'm a little on the fence regarding the idea of using the flatter hierarchy in the SVG - In general I think that flat is better than nested, and cognitively I can see how it might make sense to have the properties be attributes of the figurefirst:axis node.

That said, the pattern we have for most of the other 'additional function tags' is to add them as their own node directly under the svg node they apply to. I think that might give a little better separation of concerns such that the nesting is like:

SVG geometry/
├──figurefirst:object_type_indicator
├──figurefirst:function1_indicator
│   └──figurefirst:function1_arg
└── figurefirst:function2_indicator
    └──figurefirst:function2_arg

Where function1 and function1 map to functions that would be applied to the figurefirst object type. I'm not sure we adopt this all cases, but I think the majority.

In any case, if we add the function, we should probably also create an extension to decorate the SVG since the original motivation for the library was to create a UI for matplotlib layout design. It's been a while but part of the reason for adopting the above strategy might have related to the ease of writing the extensions.

Curious what others think. A PR would be very welcome!

@alchem0x2A
Copy link
Author

Thanks for the prompt reply!

I believe the correct hierarchy should have the figurefirst:mplmethods node under the svg rect node as is generated by the extensions.

Indeed. I got the correct way now by using the inkscape extension. Interesting the code seems quite flexible so the nested node also works.

In general I think that flat is better than nested, and cognitively I can see how it might make sense to have the properties be attributes of the figurefirst:axis node.

I think your proposal is cool, although I'm not 100% sure how to distinguish the object_type_indicator and the function2_indicator during parsing. I'm not familiar with the svg grammar, so wonder if something like semicolon-separated namespace like figurefirst:methods:some_function_handler will work.

In any case, if we add the function, we should probably also create an extension to decorate the SVG since the original motivation for the library was to create a UI for matplotlib layout design. It's been a while but part of the reason for adopting the above strategy might have related to the ease of writing the extensions.

That's true. If adhering to the API of 0.0.6 the proof-of-concept approach can be to create a node figurefirst:properties parallel to the figurefirst:axis which stores the key:value pairs, and also a similar plugin like the mplmethods.

@psilentp
Copy link
Member

psilentp commented Jun 6, 2020

That's true. If adhering to the API of 0.0.6 the proof-of-concept approach can be to create a node figurefirst:properties parallel to the figurefirst:axis which stores the key:value pairs, and also a similar plugin like the mplmethods.

I think this is the general organization I was trying to suggest - we haven't really specified the figurefirst grammar anywhere in particular -- so the hierarchy I indicated above was mostly an attempt to generalize what I think is the most common pattern. Perhaps just following how the extensions tag things is the best idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement User Documentation Documentation outside the codebase.
Projects
None yet
Development

No branches or pull requests

2 participants