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

Line duplicate points fix #979

Merged
merged 4 commits into from Feb 23, 2015

Conversation

@chaosphere2112
Copy link
Contributor

@chaosphere2112 chaosphere2112 commented Jan 23, 2015

When using lines in vcs, a bug would add many duplicate points to the line object. Here's a sample:

import vcs

x = vcs.init()

l = x.createline()

l.x = [[0, .5], [0, .75], [0, 1]]
l.y = [[0, .5], [0, .75], [0, 1]]

print l.x
print l.y

x.plot(l, bg=1)

print l.x
print l.y

which outputs

[[0, 0.5], [0, 0.75], [0, 1]]
[[0, 0.5], [0, 0.75], [0, 1]]
[[0, 0.5, 0.5], [0, 0.75, 0.75], [0, 1, 1]]
[[0, 0.5, 0.5], [0, 0.75, 0.75], [0, 1, 1]]

I spruced up the prepLine function in vcs2vtk.py, and renamed the ambiguous variables that led to the bug. I also added a test that makes sure that the X and Y points are being extended correctly.

for a in [x,y]:
while len(a)<n:
while len(a)<number_points:
a.append(a[-1])
Copy link
Contributor

@allisonvacanti allisonvacanti Jan 26, 2015

@doutriaux1 This looks like the same issue we had in 46105aa. Can you check the similar functions in this file and make sure they're treating both n and N correctly?

I think this while len(a) < number_points: a.append(a[-1]) bit should just be assert(len(a) == number_points)(or similar failure) here, too, unless this is an expected feature. Otherwise it would likely just mask user mistakes and give unexpected incorrect results.

Copy link
Contributor

@doutriaux1 doutriaux1 Jan 26, 2015

Well I'm open to this, but historically it would pad it for the user with the last value.

Copy link
Contributor

@allisonvacanti allisonvacanti Jan 26, 2015

Gotcha. As long as it's doing what they'd expect it to!

Copy link
Contributor

@allisonvacanti allisonvacanti Jan 26, 2015

Oops, and I'd linked the wrong commit earlier -- I meant to point at this one: d8f725a

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Feb 21, 2015

is this still valid?

@chaosphere2112
Copy link
Contributor Author

@chaosphere2112 chaosphere2112 commented Feb 23, 2015

Yup. Looks like it's still doing it.

In [1]: l = vcs.createline()

In [2]: l.x = [[0, .5], [0]]

In [3]: l.y = [[0, .5], [0]]

In [4]: x.line(l)
Out[4]: <vcs.displayplot.Dp at 0x111a548c0>

In [5]: l.x
Out[5]: [[0, 0.5], [0, 0]]

In [6]: l.y
Out[6]: [[0, 0.5], [0, 0]]

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Feb 23, 2015

@chaosphere2112 can you also please add the test to CMakeList.txt in testing/vcs ?

Thanks,

C.

@chaosphere2112
Copy link
Contributor Author

@chaosphere2112 chaosphere2112 commented Feb 23, 2015

Oh, yeah, that would probably be helpful 😅

@doutriaux1 doutriaux1 merged commit 30c9f75 into CDAT:master Feb 23, 2015
1 check failed
@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Feb 23, 2015

dont' worry I did it

@chaosphere2112
Copy link
Contributor Author

@chaosphere2112 chaosphere2112 commented Feb 23, 2015

Ah, thanks!

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

Successfully merging this pull request may close these issues.

None yet

4 participants