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

Make OptsSpec parser more robust #270

Closed
philippjfr opened this issue Sep 16, 2015 · 11 comments
Closed

Make OptsSpec parser more robust #270

philippjfr opened this issue Sep 16, 2015 · 11 comments

Comments

@philippjfr
Copy link
Member

The OptsSpec parser for the IPython magic works well for simple types of data but often breaks when you try to pass it more complex data such as dictionaries with string values string keys and values. It is probably a larger project to handle cases like this and won't be tackled any time soon. This issue should be seen as a place to gather examples where the parser gets things wrong so we have a bit more data to work with when someone decides to look at this.

One example was highlighted in issue #262:

%%opts Scatter [fontsize={'title':' '10pt'}]
hv.Scatter(((1,2,3),(1,10,100)), group='A')
@vascotenner
Copy link
Contributor

Can support for comment be added?

%%opts Scatter [logy=True] #[logx=False]

@jlstevens
Copy link
Contributor

Support for comments would be fairly trivial although it means the # character can't be used anywhere as everything after it would be considered a comment. I would have to think a bit of the value of supporting comments versus the value of being able to use # when necessary.

Alternatively, I can imagine that there could be a different comment syntax (e.g ## ) to make such clashes less likely. Right now I am undecided on this issue.

@jbednar
Copy link
Member

jbednar commented Sep 17, 2015

Ooh, that's tricky. Using a different comment character would be confusing for people, so I would hope that's not necessary. What about allowing # only if it is escaped (\#)? That way it could still be used if that were ever important...

@jlstevens
Copy link
Contributor

What about allowing # only if it is escaped (#)? That way it could still be used if that were ever important...

That seems like a fairly reasonable approach that could be implemented quite easily...

@maxalbert
Copy link
Contributor

@jlstevens In which positions do you expect # to appear if it wasn't to introduce a comment? Would it only be inside strings? In that case it sound like with a sufficiently flexible parser you should be able to easily ignore # when it occurs inside strings and otherwise treat it as starting a comment. But I may of course be missing something.

@maxalbert
Copy link
Contributor

Btw, I hit the same problem which @philippjfr mentioned above, but with a number instead of a string as the value (using the matplotlib backend):

%%opts Curve [fontsize={'xlabel': 30}]
Curve([(1, 1), (2, 2)])

Without having looked at the code it appears to me that the options parser is trying to re-implement parsing of basic Python syntax, but in a somewhat ad-hoc way which misses some corner cases. Is there any value in using ast.literal_eval() for this (see here for documentation of the ast module)?

@jlstevens
Copy link
Contributor

the options parser is trying to re-implement parsing of basic Python syntax, but in a somewhat ad-hoc way which misses some corner cases.

That is true for some parts of the syntax where Python literals are involved. This is due to the need for pyparsing for the non-standard portions but yes, using ast.literal_eval() does seem like a very good suggestion that I will have to investigate.

@philippjfr
Copy link
Member Author

As reported by @vascotenner in #1035, the parser does not like floats without a value before the decimal points:

Works:

%%opts HLine (linewidth=1, color=(1,1,1,0.5))

Doesn't work:

%%opts HLine (linewidth=1, color=(1,1,1,.5))

@philippjfr
Copy link
Member Author

philippjfr commented Feb 13, 2017

Also as described by @vascotenner in #1105, the parser is not using the future float division even if you define it. I think for it would be fine if we imported future division in the parser module. Example of the issue:

from __future__ import division
assert 0.75 == 3/4

%%opts Raster [aspect=3/4]
hv.Raster(np.random.randint((10,20))

ZeroDivisionError

@philippjfr
Copy link
Member Author

Another small bug encountered it seems the parser breaks completely when the specification contains spaces, e.g.:

%%opts Layout [shared_axes = False]

/home/fz/anaconda3/lib/python3.5/site-packages/holoviews-1.7.dev8-py3.5.egg/holoviews/ipython/parser.py in parse(cls, line, ns)
333 if 'plot_options' in group:
334 plotopts = group['plot_options'][0]
--> 335 opts = cls.todict(plotopts, 'brackets', ns=ns)
336 options['plot'] = {cls.aliases.get(k,k):v for k,v in opts.items()}
337
/home/fz/anaconda3/lib/python3.5/site-packages/holoviews-1.7.dev8-py3.5.egg/holoviews/ipython/parser.py in todict(cls, parseresult, mode, ns)
96 joiner=',' if any(((')' in el) or ('}' in el))
97 for el in elements) else ''
---> 98 grouped[-1] += joiner + joiner.join(elements)
99
100 for keyword in grouped:
IndexError: list index out of range

@philippjfr
Copy link
Member Author

We have pretty much deprecated the magics at this point and do not recommend using the string parser so I'm going to close this. Improving the parser proved to be pretty much impossible.

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

No branches or pull requests

5 participants