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 offset new attribute #2069

Merged
merged 16 commits into from Aug 4, 2016

Conversation

Projects
None yet
3 participants
@doutriaux1
Member

doutriaux1 commented Jul 29, 2016

@danlipsa a few baselines are still missing (Linux) it is ready for review.

goes with: CDAT/uvcdat-testdata#149

@kwrobot

This comment has been minimized.

Member

kwrobot commented Jul 29, 2016

Basic content checks failed!

commit 7b37eb85 adds bad whitespace:
testing/vcs/test_vcs_legend_2.py:55: new blank line at EOF.

commit 221e44ee adds bad whitespace:
Packages/vcs/vcs/template.py:1765: trailing whitespace.
+                    startLength, 
Packages/vcs/vcs/template.py:1770: trailing whitespace.
+                        startThick, 
Packages/vcs/vcs/template.py:1772: trailing whitespace.
+                        startThick + thick, 
testing/vcs/test_vcs_legend.py:55: new blank line at EOF.

commit 58e6d05b adds bad whitespace:
Packages/vcs/vcs/template.py:1741: trailing whitespace.
+        L = [] # length 

Branch-at: 63ba8e8
Rejected-by: @kwrobot

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jul 29, 2016

@danlipsa @aashish24 @mcenerney1 this is needed for ACME improvement. Please review.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Aug 1, 2016

@doutriaux1 @aashish24 Why do so many baselines change? What is offset? I would squash related commits. A commit such as 'pep8ed and flake8ed' has no value in history - it only create noise.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Aug 2, 2016

@danlipsa so many baseline changes because the legend is slightly different now. offset is the distance of the legend labels from the colorbar. peped8 and such commits have value to me because they are cosmetic changes and they make it harder to follow the changes that actually happened in the process. Going commit by commit helps reviewing and we can happily skip the pep8 ones. I personally don't like PR with ONE commit, I like to see the work that went in nd follow the thought process. History is mostly useless once a PR is in anyway.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Aug 2, 2016

@doutriaux1 I agree that adding extra commits makes reviewing easier - this is one place many commits is better than a squashed commit. We use the history in ParaView and VTK to track regressions that would otherwise take longer to fix.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Aug 2, 2016

@danlipsa I see. Feel free to squash commits if it really bothers you.

@property
def offset(self):
"""offset position in % of page (can be negative)"""
return self._offset

This comment has been minimized.

@danlipsa

danlipsa Aug 2, 2016

Contributor

@doutriaux1 Can you expand the comment for offset to include the detailed explanation in the comments section?

@@ -115,6 +120,8 @@ def __init__(self, member):
self.y1 = 0.129999995232
self.x2 = 0.949999988079
self.y2 = 0.159999996424
self.offset = .015

This comment has been minimized.

@danlipsa

danlipsa Aug 2, 2016

Contributor

@doutriaux1 Would it be possible to have a smaller value here so that most baselines do not change. It seems that for the first images at least, the change is the bigger offset?

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Aug 2, 2016

@doutriaux1 Besides my comments, the changes look reasonable.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Aug 2, 2016

@doutriaux1 I can live without squashed commits :-) I thought it might help us if we need to go back to a previous valid version. But maybe this is more than we need for uvcdat.

@kwrobot

This comment has been minimized.

Member

kwrobot commented Aug 2, 2016

Basic content checks failed!

commit ac8580af has committer name "GitHub" with no space.  Run
  git config --global user.name 'Your Name'
  git config --global user.email 'you@yourdomain.com'
before creating commits.
commit 7b37eb85 adds bad whitespace:
testing/vcs/test_vcs_legend_2.py:55: new blank line at EOF.

commit 221e44ee adds bad whitespace:
Packages/vcs/vcs/template.py:1765: trailing whitespace.
+                    startLength, 
Packages/vcs/vcs/template.py:1770: trailing whitespace.
+                        startThick, 
Packages/vcs/vcs/template.py:1772: trailing whitespace.
+                        startThick + thick, 
testing/vcs/test_vcs_legend.py:55: new blank line at EOF.

commit 58e6d05b adds bad whitespace:
Packages/vcs/vcs/template.py:1741: trailing whitespace.
+        L = [] # length 

Changes since last check: compare

Branch-at: ac8580a
Rejected-by: @kwrobot

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Aug 3, 2016

@danlipsa this is ready for final review all test pass on my mac and linux, and they should all pass on travis once this is merged in fix_travis.I can merge in master.

while [[ `which python` != *"@CONDA_ENVIRONMENT_NAME@"* ]]
do
source activate @CONDA_ENVIRONMENT_NAME@
done

This comment has been minimized.

@danlipsa

danlipsa Aug 4, 2016

Contributor

@doutriaux1 Why do you need to do this?

This comment has been minimized.

@doutriaux1

doutriaux1 Aug 4, 2016

Member

@danlipsa because sometimes the environement does not get activated. I suspect some concurency issues within conda, too many process trying to activate at same time, so I keep trying until it's actually activated. We could/should probably improve the test to see if it's activated (using conda env list or something like that) but that would be another PR.

This comment has been minimized.

@danlipsa

danlipsa Aug 4, 2016

Contributor

@doutriaux1 I see. Thanks!

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Aug 4, 2016

@doutriaux1 LGTM 👍

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Aug 4, 2016

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Aug 4, 2016

not convinced. If it's for the reviewer that cares for the "net effect" then just look at this on the PR. Now if that is not enough and the reviewer wants to look at the history in detail then it's easy better I think to ignore the pep8ed commit and look at the previous one which has only the changes made, rather than a bi commit with a bunch of useless pep8ed changes in it that had to what really changed. Anyhow, feel free to squash them if you feel like the history would be better.

@doutriaux1 doutriaux1 merged commit eb89153 into master Aug 4, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@doutriaux1 doutriaux1 deleted the legend_offset_new_attribute branch Aug 4, 2016

aashish24 added a commit that referenced this pull request Aug 7, 2016

Legend offset new attribute (#2069)
* gfortran not needed any longer on mac

* added offset attriubte

* ok legend now works

* added a bunch of tests, make sure line is drawn once only

* got legend_2 to work

* pep8ed and flake8ed

* Update VCS_validation_functions.py

* get text offset a bit smaller

* huge bug in extension and levels setting, fixed now

* updated correct json file to reflect new attributes

* updated correct json file to reflect new attributes

* apparently matplotlib cmaps need cycler

* reverted update baselines to False and cleaned up styling

* seems like sometimes activating fails, sticking th activation in a loop

I suspects the fail is due to some concurency issue so repeating for a while should eventually work

* adding clean-local to cmake
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment