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

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

Choose a reason for hiding this comment

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

@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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@aashish24
Copy link
Contributor

is this still valid?

@chaosphere2112
Copy link
Contributor Author

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

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

Thanks,

C.

@chaosphere2112
Copy link
Contributor Author

Oh, yeah, that would probably be helpful 😅

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

dont' worry I did it

@chaosphere2112
Copy link
Contributor Author

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
Development

Successfully merging this pull request may close these issues.

4 participants