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

Fix stairs and area legend #602

Merged
merged 5 commits into from Apr 8, 2015
Merged

Fix stairs and area legend #602

merged 5 commits into from Apr 8, 2015

Conversation

okomarov
Copy link
Member

@okomarov okomarov commented Apr 7, 2015

Fixes #601 - stairSeries and areaSeries not toggling off legend entries if not in the legend.

@okomarov okomarov added the +0.0.1 label Apr 7, 2015
@PeterPablo
Copy link
Member

Changes look good, judging by the code. Travis stays green as well. To avoid regressions, it would be good to adapt an existing ACID test so that this behavior is covered. What do you think?

@okomarov
Copy link
Member Author

okomarov commented Apr 7, 2015

AAAAAArrrrrghhhhh!!! The Octave hash for stairs fails.

I need some input so that maybe we can fix it in a easy way @egeerardyn, @PeterPablo?

Would you know off the top of your head where are the newlines problems coming from and why BEGIN/END file headers are missing? Also, maybe we could call the whole ACID with a stricter float format, e.g. '%8.3', or truncate small values to 0 (I know this one is dangerous, but it might work for our subset of tests)?

capture

@PeterPablo
Copy link
Member

I am currently on the road. Did you run 'testHeadless' in your octave?

@okomarov
Copy link
Member Author

okomarov commented Apr 7, 2015

@PeterPablo Yes, that's how I produced the new hash.

@egeerardyn
Copy link
Member

@okomarov The begin/end headers are something that are added in execute_type_stage and they are not supposed to end up in the actual files. They are just a way to delimit the files being typed to stdout (e.g. to check if there are leading/trailing lines). So there is nothing wrong with that part.

I think it's a bit weird that the EOLs are a problem only at those locations. We use \n everywhere, which should produce a LF character on any platform and should cause similar errors everywhere or nowhere. Not only at some places as you show...

The numerical format is indeed also a problem: I already reduced it to %8.6g for exactly that reason. But maybe that is still too much (I don't mind reducing it even further, but it should be done in a separate PR). The difference in format in the exponent worries me even more, since that is not specified by us, but apparently has OS-dependent settings. If we can specify that part, we should.

@PeterPablo
Copy link
Member

PeterPablo commented Apr 7, 2015 via email

@okomarov
Copy link
Member Author

okomarov commented Apr 8, 2015

My bad for the newlines, I just copy-pasted from https://travis-ci.org/matlab2tikz/matlab2tikz/builds/57505018 instead than from the log.

So, the problem is only numerical.

@PeterPablo
Copy link
Member

Unfortunately I can confirm the numerical difference between Windows Octave and Mac Octave. Mac behaves like Travis and Windows shows the same different digit like yours. As such, I would say, please commit the Hash that Travis expects. The test will thus pass on Travis, but not for you locally. We should then look into resolving this problem, e.g. by further reducing the output format -- as you suggested.

Here are my findings concerning eps:
octave MAC:

octave:3> eps
ans =    2.2204e-16

octave WIN:

>> eps
ans =   2.2204e-016

MATLAB R2014b MAC:

>> eps
ans =
   2.2204e-16

So at least eps can be regarded as constant and we could thus exploit it, to come by those numerical issues.

@PeterPablo
Copy link
Member

Look, what I found here. This makes the win output look like:

1        2.44921e-16\\
2        2.44921e-16\\

-- no more leading zero in the exponent :-) The last decimal is still wrong and as such we should adapt the floatFormat in testHeadless.

PeterPablo added a commit to PeterPablo/matlab2tikz that referenced this pull request Apr 8, 2015
'%+08.3f' ensures:
* leading sign
* same position of decimal point
* less precission to prevent possible issues between different environments (see PR matlab2tikz#602 for discussion)
@PeterPablo PeterPablo mentioned this pull request Apr 8, 2015
PeterPablo added a commit to PeterPablo/matlab2tikz that referenced this pull request Apr 8, 2015
'%+08.3f' ensures:
* leading sign
* same position of decimal point
* less precission to prevent possible issues between different environments (see PR matlab2tikz#602 for discussion)

remove leading zero and sign

leading zeros caused uncompilable `0000+nan`.
PeterPablo added a commit to PeterPablo/matlab2tikz that referenced this pull request Apr 8, 2015
'%+08.3f' ensures:
* leading sign
* same position of decimal point
* less precission to prevent possible issues between different environments (see PR matlab2tikz#602 for discussion)

remove leading zero and sign

leading zeros caused uncompilable `0000+nan`.
PeterPablo added a commit to PeterPablo/matlab2tikz that referenced this pull request Apr 8, 2015
'%8.3f' ensures:
* same position of decimal point
* less precision to prevent possible issues between different environments (see PR matlab2tikz#602 for discussion)

NOTE: do not add leading zeros as this will cause `000+nan` which is not parseable by pgfplots
okomarov pushed a commit that referenced this pull request Apr 8, 2015
@okomarov okomarov merged commit 1035a51 into matlab2tikz:develop Apr 8, 2015
@PeterPablo
Copy link
Member

👍

@okomarov okomarov deleted the fixStairsAndAreaLegend branch April 8, 2015 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants