Skip to content

Fix plotting CartesianPoint with correct units#551

Merged
fhagemann merged 1 commit intomainfrom
plot_points
Oct 28, 2025
Merged

Fix plotting CartesianPoint with correct units#551
fhagemann merged 1 commit intomainfrom
plot_points

Conversation

@fhagemann
Copy link
Collaborator

Quick attempt to fix #550:
CartesianPoints can now have units or be unitless.
However, when plotting, it would be nice to always have them have units.
Right now, if a CartesianPoint does not have units, there is no way that the plot recipe knows what units the coordinates should have. According to our docstrings, we expect x, y and z to be in units of meters though, so this should also be reflected in the plots or be updated.

"""
struct CartesianPoint{T} <: AbstractCartesianPoint{T}
Describes a three-dimensional point in Cartesian coordinates.
## Fields
* `x`: x-coordinate (in m).
* `y`: y-coordinate (in m).
* `z`: z-coordinate (in m).
Given a point `pt = CartesianPoint(x, y, z)`, use `pt - cartesian_zero` to get
a `CartesianVector` from the origin to point `pt`.
See also [`CylindricalPoint`](@ref).
"""

@fhagemann fhagemann added bug Something isn't working convenience Improve user-friendliness labels Oct 27, 2025
@oschulz
Copy link
Member

oschulz commented Oct 27, 2025

Looks good to me.

Why wasn't this a problem in the past?

@fhagemann
Copy link
Collaborator Author

Looks good to me.

Why wasn't this a problem in the past?

We used to multiply the internal units to the coordinates of the points, but it was removed in PR #489 in commit 71f1435#diff-95b64031ac6b998495f3633cff57e7303b8128e459b3e9bfcc24214ad67cf409L14.
So, I guess this was just an oversight by allowing for units in the first place.

This is also related to #183 (comment)

@series begin
seriestype --> :scatter
[pt.x], [pt.y], [pt.z]
[to_internal_units(pt.x) * internal_length_unit],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Quick explanation: to_internal_units will convert the value to internal_length_unit (in our case meters) and then strip the unit (or do nothing if the value does not have any units), and then add the unit again afterwards.
Therefore, we can handle both numbers with and without units.

Copy link
Member

@oschulz oschulz Oct 27, 2025

Choose a reason for hiding this comment

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

Yes, that seems very safe! I like it.

@fhagemann
Copy link
Collaborator Author

The updated documentation should be found here: https://juliaphysics.github.io/SolidStateDetectors.jl/previews/PR551 (once completed)

@oschulz
Copy link
Member

oschulz commented Oct 27, 2025

We used to multiply the internal units to the coordinates of the points, but it was removed in PR #489

Oh, oops. :-) Yes, then let's do it like this.

@oschulz
Copy link
Member

oschulz commented Oct 27, 2025

Maybe we can have a central function with_units or so for points and vectors, instead of putting the implementation into the series recipe? Might be useful in other contexts too.

@fhagemann
Copy link
Collaborator Author

I would refer this to a dedicated PR, that then also tackles everything else in #183

@oschulz
Copy link
Member

oschulz commented Oct 27, 2025

Sure.

@fhagemann
Copy link
Collaborator Author

@fhagemann fhagemann merged commit 056a94e into main Oct 28, 2025
10 checks passed
@fhagemann fhagemann deleted the plot_points branch October 28, 2025 15:33
@fhagemann fhagemann linked an issue Oct 28, 2025 that may be closed by this pull request
11 tasks
@fhagemann fhagemann added this to the v0.11.0 milestone Nov 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working convenience Improve user-friendliness

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plotting CartesianPoints does not seem to be correct Release v0.11.0

2 participants