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

isofill vcs bug #1947

Closed
doutriaux1 opened this issue Apr 25, 2016 · 25 comments
Closed

isofill vcs bug #1947

doutriaux1 opened this issue Apr 25, 2016 · 25 comments

Comments

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Apr 25, 2016

using the attached file Dave was able to produce the following plot. The max is 29.xx so no data should be in the pink 30-35 interval.

isofillbugmaybe

HadSST1870to99.nc.zip

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 8, 2016

hum... I'm not sure what's going on here. I was able to reproduce exactly Dave's plot with:

import cdms2
import vcs

x=vcs.init()
f=cdms2.open("HadSST1870to99.nc")
s=f("sst")
print s.min(),s.max()
iso=x.createisofill()
iso.levels=range(-5,36,5)
x.plot(s,iso)
raw_input("Press enter")

but using:

import cdms2
import vcs

x=vcs.init()
f=cdms2.open("HadSST1870to99.nc")
s=f("sst")
print s.min(),s.max()
iso=x.createisofill()
iso.levels=range(-5,34,5)
x.plot(s,iso)
raw_input("Press enter")

I get:
good

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 8, 2016

@danlipsa @aashish24 I think I'm near the issue it seems that we don't actually care what values the user pass see this code:

            cots.append(cot)
            mapper.SetInputConnection(cot.GetOutputPort())
            lut.SetNumberOfTableValues(len(tmpColors[i]))
            for j, color in enumerate(tmpColors[i]):
                r, g, b, a = self.getColorIndexOrRGBA(_colorMap, color)
                if style == 'solid':
                    tmpOpacity = tmpOpacities[j]
                    if tmpOpacity is None:
                        tmpOpacity = a / 100.
                    else:
                        tmpOpacity = tmpOpacities[j] / 100.
                    print "DO WE COME HERE",j,r,g,b,tmpOpacity
                    lut.SetTableValue(j, r / 100., g / 100., b / 100., tmpOpacity)
                else:
                    print "OR DO WE COME HERE"
                    lut.SetTableValue(j, 1., 1., 1., 0.)
            print "WE COME HERE"
            luts.append([lut, [0, len(l) - 1, True]])
            mapper.SetLookupTable(lut)
            mapper.SetScalarRange(0, len(l) - 1)
            mapper.SetScalarModeToUseCellData()

when running I get:

DO WE COME HERE 0 0 33 100 1.0
DO WE COME HERE 1 0 100 55 1.0
DO WE COME HERE 2 57 100 0 1.0
DO WE COME HERE 3 100 51 0 1.0
DO WE COME HERE 4 100 5 0 1.0
DO WE COME HERE 5 39 0 0 1.0
DO WE COME HERE 6 73 0 74 1.0

I don't see how -5, 0, 5, etc... gets mapped via

            mapper.SetScalarRange(0, len(l) - 1)
            mapper.SetScalarModeToUseCellData()

what if the levels are not equally spaced -5,0,6,7,15 ?
I think this might be a VERY serious bug.

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 8, 2016

code is in isofillpipeline.py

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 8, 2016

even more serious that the bg=1 memory leak.

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 9, 2016

furthermore the follong code produces the same plt no matter except for the legend

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 9, 2016

import cdms2
import vcs

x=vcs.init()
f=cdms2.open("HadSST1870to99.nc")
s=f("sst")
print s.min(),s.max()
iso=x.createisofill()
iso.levels=[-15,-14,-13,-12,-11,-10,-9,-8,-7,-6,35]
x.plot(s,iso)

good2

Loading

@danlipsa
Copy link
Contributor

@danlipsa danlipsa commented Jun 9, 2016

@doutriaux1 Sounds good. I will take a look at this.

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 9, 2016

import cdms2
import vcs

x=vcs.init()
f=cdms2.open("HadSST1870to99.nc")
s=f("sst")
print s.min(),s.max()
iso=x.createisofill()
iso.levels=[-15,-10,-5,0,5,10,15,20,25,30,35]
x.plot(s,iso)

good

Loading

doutriaux1 added a commit that referenced this issue Jun 9, 2016
@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 9, 2016

@danlipsa thanks I started to fix it in: issue_1947_isofill_off

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 9, 2016

@danlipsa I will check boxfill/custom as well.

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 9, 2016

@danlipsa the issue at hand here is that we use

             mapper.SetLookupTable(lut)
-            mapper.SetScalarRange(0, len(l) - 1)
-            mapper.SetScalarModeToUseCellData()
+            mapper.SetScalarRange(l[0],l[-1])
+            #mapper.SetScalarModeToUseCellData()
             mappers.append(mapper)

So we use a range, whereas we need to tell VTK the bands of each color. That might mean creating multiple mappers. That will allow us to unify the code, we have a special section if each "level" has a different pattern, maybe we ought to go through that code no matter what. Is it really bad to create many mappers?

Loading

@danlipsa
Copy link
Contributor

@danlipsa danlipsa commented Jun 9, 2016

Is it really bad to create many mappers?

It can be less than optimal, if you can get the same picture by manipulating the color map.

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 9, 2016

@danlipsa I'm not following if you use scalarrange you're assuming the distance between each level is identical no? To use scalar range we would need to create many levels using the smallest common delta between all levels. No?

Loading

doutriaux1 added a commit that referenced this issue Jun 9, 2016
@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 9, 2016

@danlipsa I think I have a fix in that branch, do you mind taking a look and see if it makes sense?

Loading

@danlipsa
Copy link
Contributor

@danlipsa danlipsa commented Jun 9, 2016

@doutriaux1 Yes, it does make sense. I will comment on it once you make the PR.

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 9, 2016

@danlipsa thanks running ctest to make sure no baselines have changed.

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 9, 2016

Loading

@danlipsa
Copy link
Contributor

@danlipsa danlipsa commented Jun 10, 2016

@doutriaux1 I think this is point vs cell data. We might _needCellData = True. One of those filters might need point data so we might need to convert along the way somewhere. You want me to take over?

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 10, 2016

yes please. Also yesterday with @sampsonbryce I tried to reproduce the bug using clt.nc and using [0,1,2,3,4,5,6,7,8,9,100] for levels and it worked! That really confused me.

Loading

@danlipsa
Copy link
Contributor

@danlipsa danlipsa commented Jun 10, 2016

@doutriaux1 What is the rule if the scalar range extends beyond your min/max level? In the sample pictures you posted, areas that have scalar than -15 are black, but there are tests where those areas are white.
https://github.com/UV-CDAT/uvcdat-testdata/blob/master/baselines/vcs/test_vcs_legend_isofill_horizontal_ext1_n_ext2_n.png

Also, I recently fixed a bug where missing attributes where filled with the closed value:
#2005

So, should we do the same here? Should areas with values less than -15 be blue instead of black?

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 10, 2016

@danlipsa I think the blacks in this case are actual missing values. data outside the range should not be drawn at all (i.e white) unless of course there's an extension arrow.

Loading

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jun 10, 2016

@danlipsa that's why in my the branch I change the to level[0] and level[1] rather than 0/1 before. I also made it create a new mapper if the delta is not the same. I don't see why these two changes triggered the blurriness.

Loading

@danlipsa
Copy link
Contributor

@danlipsa danlipsa commented Jun 10, 2016

@doutriaux1 Sounds good then. Data outside the range specified by the levels should be white.

Loading

@danlipsa
Copy link
Contributor

@danlipsa danlipsa commented Jun 10, 2016

and data masked (missing values) should be black.

Loading

danlipsa added a commit that referenced this issue Jun 12, 2016
When smallest level is bigger than min scalar value or biggest level
is smaller than max scalar value isofill creates the wrong image.
danlipsa added a commit that referenced this issue Jun 15, 2016
When smallest level is bigger than min scalar value or biggest level
is smaller than max scalar value isofill creates the wrong image.
danlipsa added a commit that referenced this issue Jun 15, 2016
When smallest level is bigger than min scalar value or biggest level
is smaller than max scalar value isofill creates the wrong image.

Also, out of range (white color) was shown black.
danlipsa added a commit that referenced this issue Jun 15, 2016
When smallest level is bigger than min scalar value or biggest level
is smaller than max scalar value isofill creates the wrong image.

Also, out of range (white color) was shown black.
danlipsa added a commit that referenced this issue Jun 15, 2016
When smallest level is bigger than min scalar value or biggest level
is smaller than max scalar value isofill creates the wrong image.
danlipsa added a commit that referenced this issue Jun 15, 2016
When smallest level is bigger than min scalar value or biggest level
is smaller than max scalar value isofill creates the wrong image.

Also, out of range (white color) was shown black.
danlipsa added a commit that referenced this issue Jun 21, 2016
BUG #1947: isofill does not handle out of bounds levels correctly.
@danlipsa
Copy link
Contributor

@danlipsa danlipsa commented Jun 30, 2016

See the following PR:
#2027

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants