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

Fixes doubled colors in legend and reduces label count #1895

Merged
merged 10 commits into from
May 13, 2016
Merged

Conversation

chaosphere2112
Copy link
Contributor

Fixes the issues outlined in #1894; still need to add a test.

@aashish24
Copy link
Contributor

@chaosphere2112 when you add a test could you please reopen this again?

@aashish24 aashish24 closed this Apr 6, 2016
chaosphere2112 pushed a commit to CDAT/uvcdat-testdata that referenced this pull request Apr 6, 2016
@chaosphere2112 chaosphere2112 reopened this Apr 6, 2016
@aashish24
Copy link
Contributor

@chaosphere2112 can you resolve the merge conflicts?

@chaosphere2112
Copy link
Contributor Author

@aashish24 Yeah; there's another bug that I ran into that I'm running down right now. I'll get an update for this by EOD.

@aashish24
Copy link
Contributor

Sure, thanks.

@chaosphere2112
Copy link
Contributor Author

@aashish24 I ran into a few more issues, I'll clean this up in the morning.

@chaosphere2112
Copy link
Contributor Author

@doutriaux1 @aashish24

Here's where this PR stands currently on CDASH.

So this is starting to get a little bit out of hand. I've done some major improvements to some of the baselines (test_vcs_boxfill_zero and test_vcs_boxfill_lev1_lev2 are notably better), but then there's... debatable movement on others (test_vcs_boxfill_lev1_lev2_ext1 is a good example of one that isn't bad, but it's certainly not better than it was). I think I fixed a bug in test_vcs_issue_960 (it looks like the last label was missing from the legend, for some reason). How do people feel about less "pretty" numbers for some scales? I can try and be a little smarter about reverting to the old method (vcs.mkscale), but my first attempt at doing so made test_vcs_boxfill_lev1_lev2 revert back to the old legend.

@aashish24
Copy link
Contributor

@chaosphere2112 less pretty numbers you meant they are decimans now? I like the new legend but I think it would be great to have an option to clamp to round numbers since those are easy to read but in some cases you may not want that.

@chaosphere2112
Copy link
Contributor Author

@aashish24 The users still have the ability to use round numbers; if they don't specify level_1 and level_2 on the boxfill, it'll behave exactly the same as it always has (which is why I'm not failing more baselines 😉 ). The "less pretty" numbers are a kind of weird artifact of how I'm generating the labels; looking at the code, I'm guessing there's an off-by-one error somewhere that I've missed which is responsible for an extra label making the levels be shifted off of the "nice" values.

@chaosphere2112
Copy link
Contributor Author

@aashish24 @doutriaux1 Alright, this is ready. I fixed the off-by-one errors, so now the same output should be generated for the same input, plus improvements (like a missing level, and lining up the last level, and other goodies).

@chaosphere2112
Copy link
Contributor Author

@aashish24 ping

@aashish24
Copy link
Contributor

thanks @chaosphere2112 will look into it today.

low_end = numpy.ma.log10(low_end)
high_end = numpy.ma.log10(high_end)


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chaosphere2112 nitpick, do we need this many empty lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I'll remove the extra one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aashish24
Copy link
Contributor

LGTM 👍

@chaosphere2112 chaosphere2112 merged commit 5ba2503 into master May 13, 2016
@doutriaux1
Copy link
Contributor

@chaosphere2112 don't forget to delete branch after merging

@doutriaux1 doutriaux1 deleted the fix_1894 branch May 13, 2016 06:44
@chaosphere2112
Copy link
Contributor Author

@doutriaux1 Sorry, got excited and merged when I was going to bed 😄

@doutriaux1
Copy link
Contributor

you mean you actually take time to sleep! 😜

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

Successfully merging this pull request may close these issues.

3 participants