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 pretty.names = FALSE in plotThreshVsPerf #1168

Merged
merged 1 commit into from
Aug 18, 2016
Merged

fix pretty.names = FALSE in plotThreshVsPerf #1168

merged 1 commit into from
Aug 18, 2016

Conversation

zmjones
Copy link
Contributor

@zmjones zmjones commented Aug 18, 2016

fix for #1167

@larskotthoff
Copy link
Sponsor Member

Could you make the test an additional one please so that pretty names are also tested?

@zmjones
Copy link
Contributor Author

zmjones commented Aug 18, 2016

so i modified a test so that this bug would have caused a failure. you would prefer that i duplicate the test with only pretty.names changed so the failure would be unambiguous? i think the error message would have been easy to diagnose in this case.

@larskotthoff
Copy link
Sponsor Member

Yes. In particular the modified test doesn't test what happens for the other value of pretty names :)

@zmjones
Copy link
Contributor Author

zmjones commented Aug 18, 2016

No but since it is the default it is checked in all of the other calls to plotThreshVsPerf. I don't think in this particular case there is any advantage to duplicating the test since this part of the function is the same across all the other args that are being varied in the rest of the tests.

However the cost to adding another call to the plot function is small, so I can add it if you would really like it :)

@larskotthoff
Copy link
Sponsor Member

Ok, fair enough.

@larskotthoff larskotthoff merged commit 2e52b97 into master Aug 18, 2016
@mllg mllg deleted the 1167 branch September 6, 2016 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants