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

Compute vector scaling correctly #1988

Merged
merged 19 commits into from May 31, 2016

Conversation

Projects
None yet
3 participants
@aashish24
Contributor

aashish24 commented May 23, 2016

VTK does clamping and not remapping of input range to desired
range which leads to undesirable visual effects.

@aashish24 aashish24 force-pushed the fix_vector_scale branch from 7836cd2 to 2095654 May 23, 2016

Compute vector scaling correctly
VTK does clamping and not remapping of input range to desired
range which leads to undesirable visual effects.

@aashish24 aashish24 force-pushed the fix_vector_scale branch from 2095654 to 1129e5b May 23, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 23, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 23, 2016

@danlipsa @doutriaux1 please review

@aashish24 aashish24 changed the title from [WIP] Compute vector scaling correctly to Compute vector scaling correctly May 23, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 23, 2016

Ref #1977

# [min, max] --> [0, 1].
glyphFilter.ClampingOn()
glyphFilter.SetRange(0.01, 1.0)

This comment has been minimized.

@danlipsa

danlipsa May 23, 2016

Contributor

It seems the original problem was triggered by this code. By clamping everything to 1, you end-up with vectors of the same size. I'll post an image generated with this code commented out.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 23, 2016

The scaling factor is not quite right, but scaling does happen. This is test_vcs_basic_vector.png
test_vcs_basic_vector

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 23, 2016

I love the flies swarming look to this!

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 24, 2016

@danlipsa yes, all of these problems are fixed in my PR. Please have a look at the new baselines and my commit message on the source of the problem.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 24, 2016

@aashish24 Why do you need to compute another scalar? You could use ScaleByVector isn't it?

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 24, 2016

@aashish24 @doutriaux1 After a discussion with @aashish24 this LGTM as long as users know/are OK with a vector magnitude range remapping to hard-coded [0.1:1]. In the future we'll add options to vcs to show data as is, clamp it low and high, use linear rescalling (what @aashish24 does), use logarithmic rescalling.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 24, 2016

@danlipsa and I had a good conversation about it and I pointed out that there are few API parameters are missing at this point. I would like to propose those changes post 2.6 release. The options will be

  • An API to enable / disable clamping and option to set the clamping range, default to OFF
  • An API to enable / disable re-mapping vector magnitude range (logarithmic, linear etc.), by default to OFF

For now, this branch takes care of the vector appearing of the same size because of clamping and keeping the old look and feel for most part.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 24, 2016

Forgot to mention that I am okay with taking out the linear range mapping from this branch (or have it not exposed) too, although that means that I would need to look into the uniform scaling factor more closely.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 24, 2016

Indeed this was a good discussion - we clarified the trade-offs for each approach and a path for fixing this properly. With @doutriaux1 input we can make a choice for the 2.6 release. I am also fine with either path we choose.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 24, 2016

@aashish24 while you're at it, we need to add the legend for vectors. use template.legend.x1/y1 for the location. The legend should be arrow of constant length with a number showing the magnitude value for this length. Does this make sense?

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 24, 2016

@doutriaux1 @danlipsa I am adding few options to the graphic method, I will push my changes today.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 24, 2016

@aashish24 thanks! Let me know when I can pull the trigger on a build.

@aashish24 aashish24 force-pushed the fix_vector_scale branch 5 times, most recently from 742681d to 12dbc57 May 25, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 25, 2016

So I added an option to the vector graphics method that could be used in the script like shown below:

import cdms2
import vcs
data = cdms2.open("./../build_debug/install/share/uvcdat/sample_data/clt.nc")
v = data['v']
u = data['u']
x = vcs.init()
gv = vcs.createvector()
gv.scaletype = 'linear'
x.plot(u, v, gv)
x.interact()

You have five options to chose from (I am open for name suggestions)
('off', 'constant', 'normalize', 'linear', 'constantNNormalize', 'constantNLinear')
off --> Do not apply any scaling (scale factor or otherwise), just show the vector magnitude
constant --> Apply constant scale factor that is gm.scale only
normalize --> Apply scale by computing the maximum vector magnitude and normalize all other vectors to it
linear --> Apply scale computing using the user provided range (using scalerange) and map vector magnitudes to that range.

I have attached images for all the options below for comparison:
constantnlinear
constantnnormalize
constant
normalize
linear
off

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 26, 2016

@doutriaux1 this branch is ready for another review. Please let me know if you have better names for the options.

@@ -528,6 +535,26 @@ def _setalignment(self, value):
self._alignment = value
alignment = property(_getalignment, _setalignment)

This comment has been minimized.

@doutriaux1

doutriaux1 May 26, 2016

Member

@aashish24 I would rather the set/get function to be in VCS_Validation functions, just in case we ever need to re-use it for something else and have here

scalerange = VCS_validation_functions.scalerange

see colormap above for example.

@@ -243,6 +243,18 @@ def checkListOfNumbers(self, name, value, minvalue=None,
return list(value)
def checkValidOption(self, name, value, options):

This comment has been minimized.

@doutriaux1

doutriaux1 May 26, 2016

Member

I think you could have sed checkInStringList, it would have made it more consistent with the rest of API where you can pass string or number.
see:
https://github.com/UV-CDAT/uvcdat/blob/b69d09d59a2d07853c5f4f2fe6987610d141b07a/Packages/vcs/vcs/VCS_validation_functions.py#L1321

@@ -660,6 +691,8 @@ def list(self):
print "alignment = ", self.alignment
print "type = ", self.type
print "reference = ", self.reference
print "scaletype = ", self.scaletype
print "scalerange = ", self.scalerange

This comment has been minimized.

@doutriaux1

doutriaux1 May 26, 2016

Member

@aashish24 I don't see the code to update the json, I dont think you need to update it (it should pick it up automagcally) but can you double check that json dump of a vector graphic method now contain these attributes as well? Probably worth adding a test. Actually are the json dump test still passing?

This comment has been minimized.

@aashish24

aashish24 May 26, 2016

Contributor

good point, let me run all the tests locally here just to make sure. thanks @doutriaux1

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 26, 2016

@aashish24 I made a few comments, building locally.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 27, 2016

@aashish24 see: https://open.cdash.org/viewTest.php?onlyfailed&buildid=4385643

     79 - test_vcs_background_mode_rotate (Failed)
    354 - test_vcs_isoline_width_stipple (Failed)
    413 - test_vcs_vectors_scale_options (Failed)
    453 - test_vcs_line_patterns (Failed)
    524 - dv3d_vector_test (Failed)
    525 - dv3d_slider_test (Failed)
    526 - Hovmoller_volume_test (Failed)
    527 - dv3d_volume_test (Failed)
    528 - dv3d_constituents_test (Failed)
    529 - dv3d_surface_test (Failed)
    530 - dv3d_hovmoller_test (Failed)
    588 - diags_test_02 (Failed)
    589 - diags_test_03 (Failed)
    590 - diags_test_04 (Failed)
    591 - diags_test_41 (Failed)
    592 - diags_test_05 (Failed)
    593 - diags_test_06 (Failed)
    594 - diags_test_07 (Failed)
    595 - diags_test_08 (Failed)
    596 - diags_test_09 (Failed)
    597 - diags_test_10 (Failed)
    598 - diags_test_11 (Failed)
    599 - diags_test_12 (Failed)
    600 - diags_test_13 (Failed)
    601 - diags_test_15 (Failed)
    603 - basemap_verify_import (Failed)
@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 28, 2016

thanks @doutriaux1. test_vcs_vectors_scale_options is failing because I didn't have bg=1 but all other ones are failing not because of my branch.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 28, 2016

@aashish24 how come the test suite got dirtied then? I though your new test suite had everything cleaned up? Beside some of these test do not fail on @danlipsa branch.

@aashish24 aashish24 force-pushed the fix_vector_scale branch from 5efb441 to c8befa3 May 28, 2016

@aashish24 aashish24 force-pushed the fix_vector_scale branch from e645cf3 to e3f7ede May 28, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 28, 2016

@doutriaux1 I added the vector method to the dump json test and made sure that it is correct. Please have a look at the branch now.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 28, 2016

@doutriaux1 I don't know as some of these are not failing on my machine. I am doing a clean build of CDAT just to be sure. The DV3D tests are failing because of some key error. Some of the diagnostic tests are failing because of color map issues. Two of them might be failing because i made the bg=1 default if not specified.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 28, 2016

On master, I have these tests failing:

The following tests FAILED:
354 - test_vcs_isoline_width_stipple (Failed)
452 - test_vcs_line_patterns (Failed)
523 - dv3d_vector_test (Failed)
524 - dv3d_slider_test (Failed)
525 - Hovmoller_volume_test (Failed)
526 - dv3d_volume_test (Failed)
527 - dv3d_constituents_test (Failed)
528 - dv3d_surface_test (Failed)
529 - dv3d_hovmoller_test (Failed)
575 - CDMS_Test_multiple_formats (Failed)
587 - diags_test_02 (Failed)
588 - diags_test_03 (Failed)
589 - diags_test_04 (Failed)
590 - diags_test_41 (Failed)
591 - diags_test_05 (Failed)
592 - diags_test_06 (Failed)
593 - diags_test_07 (Failed)
594 - diags_test_08 (Failed)
595 - diags_test_09 (Failed)
596 - diags_test_10 (Failed)
597 - diags_test_11 (Failed)
598 - diags_test_12 (Failed)
599 - diags_test_13 (Failed)
600 - diags_test_15 (Failed)

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 31, 2016

@aashish24 switching to the system python from the brew one (and rebuilding uvcdat) fixed the dv3d_ tests for me. What are the generated files for the stipple tests?

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 31, 2016

@danlipsa let's file another bug issue and carry discussion on failing tests there. I will post information there.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 31, 2016

@danlipsa please see this: #1995

Also, can we merge this branch (I don't see any major objections)?

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 31, 2016

@aashish24 fine with me.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 31, 2016

thanks @danlipsa. @doutriaux1 if you have any other concern, I will make sure to address them quickly.

@aashish24 aashish24 merged commit 8a8c1e9 into master May 31, 2016

2 checks passed

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

@aashish24 aashish24 deleted the fix_vector_scale branch May 31, 2016

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 31, 2016

rebuilding from scratch, will let you know.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 31, 2016

@aashish24! Really! How quicker could I have been!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment