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

Legend plotyy #773

Merged
merged 13 commits into from Oct 10, 2015

Conversation

@okomarov
Copy link
Member

commented Sep 17, 2015

Implements legends for plotyy() and fixes #140, #399 (in Matlab for now)

You can check on Matlab ACID(20) or other plotyy examples with legend :)

TODO:

  • Fix errors
  • Octave!
  • do not use legend string in \label{%s} but some hash of the handle?
oleg komarov added 2 commits Sep 16, 2015
strcmpi(class(handle(entry)),'hggroup');
if isHggroup
isHggroupClass = strcmpi(class(handle(entry)),'hggroup');
if isHggroupClass

This comment has been minimized.

Copy link
@okomarov

okomarov Sep 17, 2015

Author Member

Simplifies check for Hggroup class

This comment has been minimized.

Copy link
@egeerardyn

egeerardyn Sep 17, 2015

Member

👍. However, you might want to add a short note to state that handle is not supported on Octave (I know this is in a MATLAB-only branch and so it shouldn't be a problem).

This comment has been minimized.

Copy link
@okomarov

okomarov Sep 17, 2015

Author Member

👍

entries = m2t.axesContainers{end}.LegendEntries;
idx = ismember(entries, h);
string = getOrDefault(entries(idx), 'displayname', '');
string = getOrDefault(h, 'displayname', '');

This comment has been minimized.

Copy link
@okomarov

okomarov Sep 17, 2015

Author Member

No need to compare against legend entries since I need to call it on PlotyyReferences too. If handle does not have legend string, it will be empty.

Oleg Komarov added 4 commits Sep 17, 2015
@PeterPablo

This comment has been minimized.

Copy link

commented on src/matlab2tikz.m in 04c9304 Sep 17, 2015

👍 (I didn't want to nitpick before...)


% Clear plotyy references
m2t.axesContainers{end}.PlotyyReferences = [];
end

This comment has been minimized.

Copy link
@PeterPablo

PeterPablo Sep 17, 2015

Member

Can there be a else case that we should detect?

This comment has been minimized.

Copy link
@okomarov

okomarov Sep 17, 2015

Author Member
else
    % Do nothing: it's gonna be a legend entry. 
    % Not a labelled reference nor a referenced entry from the main axis.
end

I can add that to make it clearer

This comment has been minimized.

Copy link
@egeerardyn
@@ -895,7 +928,7 @@ case guitypes()
% Get legend handle associated with current axis

legendhandle = [];
env = getEnvironment;
env = getEnvironment();

This comment has been minimized.

Copy link
@egeerardyn

egeerardyn Sep 17, 2015

Member

Wouldn't it make more sense to just switch getEnvironment() below? The values are cached anyway, so performance shouldn't be an issue.

This comment has been minimized.

Copy link
@okomarov

okomarov Sep 17, 2015

Author Member

👍 I probably copy-pasted from somewhere :)

This comment has been minimized.

Copy link
@PeterPablo

PeterPablo Sep 17, 2015

Member

👍 even better

This comment has been minimized.

Copy link
@okomarov

okomarov Sep 17, 2015

Author Member

I reuse env on line 953

@@ -581,7 +581,7 @@
legend boxoff
end
% =========================================================================
function [stat] = moreLegends()
function [stat] = plotyyLegends()

This comment has been minimized.

Copy link
@egeerardyn
switch getEnvironment()
case 'Octave'
try
get(h, '__plotyy_axes__');

This comment has been minimized.

Copy link
@PeterPablo

PeterPablo Sep 17, 2015

Member

When you perform get(h), is __plotyy_axes__ a field name or is it a hidden property? If the first case, we could get rid of the try ... catch statement.

This comment has been minimized.

Copy link
@okomarov

okomarov Sep 17, 2015

Author Member

hidden property, cannot test with isfield(), returns 0 always.

This comment has been minimized.

Copy link
@PeterPablo

PeterPablo Sep 17, 2015

Member

👍 you might want to add this as a comment, so that nobody spends time on trying to optimize this

@@ -48,7 +48,6 @@ markerSizes : 703bd6d9f3a26af0062adb66493f864f
markerSizes2 : d6a12826350462efeb1f2194833cb6cc
meshPlot : 219102d4595a4dad5b6d65ab610fea9e
mixedBarLine : 0eb277cfaab5c17cc6738ede28886050
moreLegends : fce1b3851870a7a2c02b6bfbf6def080

This comment has been minimized.

Copy link
@PeterPablo

PeterPablo Sep 17, 2015

Member

Why is this hash removed?

This comment has been minimized.

Copy link
@okomarov

okomarov Sep 17, 2015

Author Member

renamed to plotyyLegends

@PeterPablo

This comment has been minimized.

Copy link
Member

commented Sep 17, 2015

I added some intermediate feedback. Thanks!

oleg komarov
@egeerardyn

This comment has been minimized.

Copy link
Member

commented Sep 18, 2015

@okomarov : I think "but some hash of the handle?" in your OP would be a source of randomness, which our hash-based testing could struggle with. An (ugly) but deterministic alternative would be to have a global counter for that. The other thing I can think of, is to make a string that represents where the referenced object lives in the HG object tree (i.e. figure), so e.g. c1c3 could map to the third child of the first child of gcf.

@okomarov

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2015

I think I'll just sprintf() the absolute value of the handle with a few decimals.

@egeerardyn

This comment has been minimized.

Copy link
Member

commented Sep 18, 2015

@okomarov The problem with that is that we don't know the algorithm that MATLAB/Octave use to determine the handle number. As such, the LaTeX output might differ depending on:

  • which version is used
  • whether there are already figures open (and how many)
  • some other factors?

and that will cause all test hashes to become unreliable.

The easiest way is to introduce a global counter in m2t (like we do for PNG images and data files). It's ugly, but it should work as long as we don't change plotting orders.

edit Now that I think of it, that might require a two-pass handling. It might require us to know the ID (whatever name we give the label) even before the corresponding element has been produced in the output. I actually have no idea how pgfplots handles those dependencies. Probably after a second compile everything is fine (I hope).

edit 2 Another problem I anticipate: the namespace of \label{...} in LaTeX is global, or in other words, the different labels should be unique across a document. So probably we need to add a figure-specific part (e.g. file name without the path part) to the output such that the labels are unique.

% Create labelled references to legend entries of the main plotyy axis

% This ensures we are either on the main or secondary axis
if ~isAxisPlotyy(m2t.currentHandles.gca), return, end

This comment has been minimized.

Copy link
@miscco

miscco Sep 21, 2015

Contributor

Could you put that on separate lines. Otherwise one cannot put a break point on return.

This comment has been minimized.

Copy link
@egeerardyn

egeerardyn Sep 22, 2015

Member

I agree.

oleg komarov added 2 commits Sep 25, 2015
oleg komarov
@egeerardyn

This comment has been minimized.

Copy link

commented on src/matlab2tikz.m in 9a96491 Sep 28, 2015

This is ugly: you recalculate it again right below this. labelNum should be the current one, not the previous one.

@egeerardyn

This comment has been minimized.

Copy link

commented on src/matlab2tikz.m in 9a96491 Sep 28, 2015

I think it might be easier to introduce it similar to the other global counters: just initialize it to 0 at the beginning.

Also, I think this should not be a property of axesContainer, but rather really global to allow subplots with multiple plotyy plots.

@egeerardyn egeerardyn self-assigned this Sep 28, 2015
@egeerardyn egeerardyn removed their assignment Sep 29, 2015
function bool = isAxisMain(h)
% Check if it is the main axis e.g. in a plotyy plot

if ~isAxisPlotyy(h)

This comment has been minimized.

Copy link
@egeerardyn

egeerardyn Sep 29, 2015

Member

Just a minor thing: I think it makes more sense to do:

bool = true;
if isAxisPlotyy(h)
    ...
end

than (the current)

if ~isAxisPlotyy(h)
    bool = true;
    return
end

...

as the latter makes the control flow more obscure IMHO (for exceptional cases (e.g. invisible lines, empty plots, ...), early returns can sometimes be advantageous, but I don't think that is the case here).

@egeerardyn

This comment has been minimized.

Copy link
Member

commented Oct 10, 2015

👍 thank you for looking into this hiatus we had. Feel free to merge whenever you like.

okomarov pushed a commit that referenced this pull request Oct 10, 2015
Legend plotyy
@okomarov okomarov merged commit 5d4cee9 into matlab2tikz:develop Oct 10, 2015
2 checks passed
2 checks passed
continuous-integration/jenkins/pr Build successful. Build failure. No test results found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@okomarov okomarov deleted the okomarov:LegendPlotyy branch Oct 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.