Skip to content

Conversation

@albertomercurio
Copy link
Member

@albertomercurio albertomercurio commented Jun 8, 2025

Checklist

Thank you for contributing to QuantumToolbox.jl! Please make sure you have finished the following tasks before opening the PR.

  • Please read Contributing to Quantum Toolbox in Julia.
  • Any code changes were done in a way that does not break public API.
  • Appropriate tests were added and tested locally by running: make test.
  • Any code changes should be julia formatted by running: make format.
  • All documents (in docs/ folder) related to code changes were updated and able to build locally by running: make docs.
  • (If necessary) the CHANGELOG.md should be updated (regarding to the code changes) and built by running: make changelog.

Request for a review after you have completed all the tasks. If you have not finished them all, you can also open a Draft Pull Request to let the others know this on-going work.

Description

use LScene instead of Axis3 for Bloch Sphere rendering, in order to have more freedom.

@albertomercurio albertomercurio marked this pull request as ready for review June 8, 2025 10:50
@albertomercurio albertomercurio requested a review from ytdHuang June 8, 2025 10:50
@codecov
Copy link

codecov bot commented Jun 8, 2025

Codecov Report

Attention: Patch coverage is 97.77778% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.97%. Comparing base (826a03d) to head (5e226f5).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
ext/QuantumToolboxMakieExt.jl 97.77% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #485      +/-   ##
==========================================
- Coverage   93.98%   93.97%   -0.02%     
==========================================
  Files          50       50              
  Lines        3424     3418       -6     
==========================================
- Hits         3218     3212       -6     
  Misses        206      206              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@ytdHuang ytdHuang left a comment

Choose a reason for hiding this comment

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

Here are some parameter changes that make the sphere looks more similar with the one in qutip.

Also, can you change the default value of Bloch.font_size to 20 (remember to also change the docstring and the table in documentation) ?
Since our sphere becomes larger, I think font size 20 suites the new size of the sphere.

for (points, _) in axes
lines!(ax, points; color = axis_color, linewidth = axis_width)
for points in axes
lines!(lscene, points; color = axis_color)
Copy link
Member

Choose a reason for hiding this comment

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

How about

Suggested change
lines!(lscene, points; color = axis_color)
lines!(lscene, points; color = b.frame_color)

so that we don't need to define axis_color

]
for circle in circles
lines!(ax, circle; color = wire_color, linewidth = 1.0)
lines!(lscene, circle; color = wire_color, linewidth = 1.0)
Copy link
Member

Choose a reason for hiding this comment

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

How about

Suggested change
lines!(lscene, circle; color = wire_color, linewidth = 1.0)
lines!(lscene, circle; color = b.frame_color, linewidth = 1.0)

so that we don't need to define wire_color

Comment on lines +440 to 443
# XY, XZ circles
circles = [
[Point3f(cos(φi), sin(φi), 0) for φi in φ], # XY
[Point3f(0, cos(φi), sin(φi)) for φi in φ], # YZ
[Point3f(cos(φi), 0, sin(φi)) for φi in φ], # XZ
Copy link
Member

Choose a reason for hiding this comment

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

Although qutip doesn't draw the YZ plane circle.

I think it is better to display the three main circles together.

t_range = range(0, θ, length = 100)
arc_points = [Point3f((v1 * cos(t) + cross(n, v1) * sin(t))) for t in t_range]
lines!(ax, arc_points; color = RGBAf(0.8, 0.4, 0.1, 0.9), linewidth = 2.0, linestyle = :solid)
lines!(lscene, arc_points; color = RGBAf(0.8, 0.4, 0.1, 0.9), linewidth = 2.0, linestyle = :solid)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure whether we should handle the color here like the way in _plot_lines! or not...

But that means we need to add the fmt into Bloch.arcs field...

Copy link
Member

Choose a reason for hiding this comment

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

can think about it in the future

_draw_reference_circles!(ax)
_draw_reference_circles!(b::Bloch, lscene)
Draw the three great circles `(XY, YZ, XZ planes)` on the Bloch sphere.
Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to keep YZ plane

transparency = true,
rasterize = 3,
)
θ_vals = range(0, π, 5)[1:(end-1)]
Copy link
Member

Choose a reason for hiding this comment

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

We only need to draw pi/4, 3*pi/4.
The 0 and 2*pi/4 cases will be handled by _draw_reference_circles! (XZ- and YZ-plane)

Suggested change
θ_vals = range(0, π, 5)[1:(end-1)]
# curves of longitude
θ_vals = [1, 3] * π / 4

Copy link
Member

Choose a reason for hiding this comment

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

(better to add the comment, make it more easier to follow in the future)

end
φ_vals = range(0.0f0, π, length = n_lat + 2)
θ_curve = range(0.0f0, 2π, length = 600)
φ_vals = range(0, π, 6)
Copy link
Member

Choose a reason for hiding this comment

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

We only need to draw pi/4, 3*pi/4.
The 0 and pi cases are just north and south poles (no need to draw).
The pi/2 case will be handled by _draw_reference_circles! (YZ-plane)

Suggested change
φ_vals = range(0, π, 6)
# curves of latitude
φ_vals = [1, 3] * π / 4

Copy link
Member

Choose a reason for hiding this comment

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

(better to add the comment, make it more easier to follow in the future)

Copy link
Member

@ytdHuang ytdHuang left a comment

Choose a reason for hiding this comment

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

I will do the above minor changes in another PR

@ytdHuang ytdHuang merged commit 22efb11 into qutip:main Jun 9, 2025
14 of 16 checks passed
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.

2 participants