Skip to content

Fixing the error that arise when using uneven width and height #360

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

Merged
merged 5 commits into from
Dec 6, 2021

Conversation

zeyuyun1
Copy link
Contributor

The current code for SurfaceGeometry works only when width_segments==height_segments. For example,

surf_g = SurfaceGeometry(z=[0]*100, 
                         width_segments=nx - 1,
                         height_segments=ny - 1)

will only run when nx == ny.

Error arise when we have:

nx = 10
ny = 15
surf_g = SurfaceGeometry(z=[0]*150, 
                         width_segments=nx - 1,
                         height_segments=ny - 1)

ValueError: all the input array dimensions for the concatenation axis must match exactly, but along dimension 0, the array at index 0 has size 15 and the array at index 2 has size 10

This is because in line 81
z = np.array(self.z).reshape((nx, ny)) the reshaped dimension is reversed.

The proposed change fixed this error without interrupting any other part.

fixing the error that arise when using uneven width and height.
@vidartf
Copy link
Member

vidartf commented Dec 6, 2021

Thanks for this contribution! I tested the fix locally using the code in examples/Examples.ipynb, and while this fixes the positions, it does seem like the indices are not correct in the nx != ny case. I'll dig some more on my side to see if I can spot the error quickly.

Uncovered this bug when testing new support for non-square sizes.
DataTexture seems to insist on column-major data. As arrays are always row-major (C-order) when serialized, we switch width/height on JS side. Some examples also need updating in order to highlight this, and to stay intuitive with respect to variable names.

Also regenerates modified example.
@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2021

Codecov Report

Merging #360 (7eea4c0) into master (150ff1c) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #360   +/-   ##
=======================================
  Coverage   70.86%   70.86%           
=======================================
  Files          23       23           
  Lines         834      834           
=======================================
  Hits          591      591           
  Misses        243      243           
Impacted Files Coverage Δ
pythreejs/pythreejs.py 70.70% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 150ff1c...7eea4c0. Read the comment docs.

@vidartf vidartf merged commit a78cb57 into jupyter-widgets:master Dec 6, 2021
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.

3 participants