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

Errorous behavior in Vispy.Visuals.Markers.set_data #2564

Open
c40zAtGitHub opened this issue Dec 28, 2023 · 2 comments
Open

Errorous behavior in Vispy.Visuals.Markers.set_data #2564

c40zAtGitHub opened this issue Dec 28, 2023 · 2 comments

Comments

@c40zAtGitHub
Copy link
Contributor

In line 606-608 of markers.py, the following statement is written:

if (edge_width is not None) + (edge_width_rel is not None) != 1: raise ValueError('exactly one of edge_width and edge_width_rel ' 'must be non-None')

while in line 583-585, the method is defined as

def set_data(self, pos=None, size=10., edge_width=1., edge_width_rel=None, edge_color='black', face_color='white', symbol='o'):

An error occurs when only edge_width_rel is supplied to the set_data method while the edge_width is untouched. The sum in the if statement in this case will be 2, causing the ValueError raised instead of normal executation of the method (which is expected since the edge_width_rel is supplied).

Current issue can be dodged by explicitly supplying edge_width=None in the input.

Apparent logic for the ValueError on line 607 to be raised should be
if (edge_width is None) and (edge_width_rel is None):

Personally, I guess that the usage of edge_width should be surpressed when edge_width_rel is supplied. This can be achieved by changing code in line 610-617 to something like
if edge_width_rel is not None: edge_width_rel = np.asarray(edge_width_rel) if np.any(edge_width_rel < 0): raise ValueError('edge_width_rel cannot be negative') else: edge_width = np.asarray(edge_width) if np.any(edge_width < 0): raise ValueError('edge_width cannot be negative')

@djhoese
Copy link
Member

djhoese commented Dec 28, 2023

Looks like some of this logic has been there for 9 years or so. In the last couple years @brisvag had updated some of it. I think your suggestion to essentially swap the i edge_width_rel is not None and edge_width if statements makes sense. Would you mind submitting a pull request and then we can get more feedback from other vispy maintainers.

@c40zAtGitHub
Copy link
Contributor Author

c40zAtGitHub commented Dec 28, 2023 via email

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

No branches or pull requests

2 participants