-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add scatter3d #30
Add scatter3d #30
Conversation
The camera buttons in the first plot don't work exactly correctly. The camera is almost but not quite aligned with the axis you are looking along. This might just be a problem with numerical precision. |
I think it might be because they are lining up with the center of the box, not the origin of the rgb axes. Because the data is random, the center of the box is close to zero but not exactly zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we make sure that plots look ok on different machines given that there are so many hard coded values. I assume, you picked them based on how things look on your machine?
requirements/base.in
Outdated
@@ -1,3 +1,2 @@ | |||
matplotlib | |||
pythreejs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not needed here anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the requirement to the docs.in
. We decided pythreejs
should be a soft requirement for plopp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't it needed in the tests as well? Are these requirement files meant for users as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about the tests.... how did the CI even pass?
cam_pos_norm = np.linalg.norm(self.camera.position) | ||
box_mean_size = np.linalg.norm(box_size) | ||
self.camera.near = 0.01 * box_mean_size | ||
self.camera.far = 5.0 * cam_pos_norm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do these numbers come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as in Scipp. It is 1% of the range spanned by the data for the near
value. It's basically to say at what distance from the camera do the objects start to disappear, so it makes sense to have it as a fraction of the extent of the data points.
The same does for the far
which means when objects start disappearing when they are far away.
Those numbers have worked well for Scipp users, I think.
src/plopp/graphics/point_cloud.py
Outdated
dtype='float32')) | ||
}) | ||
|
||
pixel_ratio = 1.0 # config['plot']['pixel_ratio'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a left over from debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and no. There is no config in plopp
yet. I didn't want to mess with the pixel_ratio
just now, so I just set it to 1, and left the commented code as a reminder. I will change to a TODO.
self.geometry.attributes["color"].array = colors.astype('float32') | ||
|
||
def _update_colorbar(self): | ||
dpi = 96 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this interact with user chosen dpi for the rest of the figure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no other dpi for the rest of the figure. Pythreejs uses pixels for figure size, no dpi.
src/plopp/graphics/color_mapper.py
Outdated
def _get_cmap(name, nan_color=None): | ||
|
||
try: | ||
cmap = copy(cm.get_cmap(name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as scipp/scipp#2824
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to match scipp
src/plopp/graphics/color_mapper.py
Outdated
except ValueError: | ||
cmap = LinearSegmentedColormap.from_list("tmp", [name, name]) | ||
# cmap.set_under(config['plot']['params']["under_color"]) | ||
# cmap.set_over(config['plot']['params']["over_color"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uncomment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, no config yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to a TODO
src/plopp/functions/scatter3d.py
Outdated
The name of the coordinate that is to be used for the Y positions. | ||
z: | ||
The name of the coordinate that is to be used for the Z positions. | ||
dim: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dim
is not a good name. Maybe coord
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, dim
is not the best. Maybe pos
, or positions
, or vectors
?
Can now make 3d scatter plots using
You can also use 3 separate coordinates instead of a
vector3
coord:Additional notes / things to check:
ColorMapper
class for 2d images and 3d scatter plotsScene3d
only makesPointClouds
but in principle, it could accept more in the future? (meshes or other).Fixes #13