Fix/rdf plot (SOF-7807) #264
Conversation
📝 WalkthroughWalkthroughUpdates a dataset JSON source label and adds Pyodide-aware plotting to render RDF plots as inline PNGs in emscripten environments while preserving existing behavior elsewhere. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as plot_rdf()
participant Sys as sys (env check)
participant MPL as matplotlib / pyplot
participant IO as io.BytesIO
participant IPY as IPython.display
rect rgb(230,245,255)
Note over Caller,IPY: Pyodide / Emscripten path
Caller->>Sys: check hasattr(sys, 'emscripten')
Sys-->>Caller: True
Caller->>MPL: matplotlib.use("Agg")
Caller->>MPL: create plot (plt.figure / plotting)
MPL-->>Caller: Figure
Caller->>IO: create BytesIO buffer
Caller->>MPL: save figure to PNG into buffer
MPL-->>IO: PNG bytes
Caller->>IPY: display(Image(buffer.getvalue()))
IPY-->>Caller: inline image rendered
Caller->>MPL: plt.close()
end
rect rgb(240,250,240)
Note over Caller,MPL: Standard (non-Pyodide) path
Caller->>Sys: check hasattr(sys, 'emscripten')
Sys-->>Caller: False
Caller->>MPL: create plot and display normally (interactive backend)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
other/materials_designer/uploads/0-Ni.jsonutils/plot.py
🔇 Additional comments (4)
other/materials_designer/uploads/0-Ni.json (1)
55-61: LGTM!The source string normalization from "Materials Project" to "MaterialsProject" is a reasonable standardization. This aligns with typical identifier naming conventions that avoid spaces.
utils/plot.py (3)
1-6: LGTM!Imports are properly organized and all are used in the Pyodide rendering path.
67-68: LGTM!The space added in "Interface {i + 1}" improves readability in hover text and trace names.
84-96: The code is safe as-is. Line 82 explicitly sets matplotlib to theAggbackend when running in Pyodide, which is a non-interactive, file-based backend. In Agg mode,plt.show()becomes a no-op and does not clear the figure, so even ifplot_distribution_functioncalls it internally, the figure will remain available forplt.savefig()to capture on line 93. The backend selection before plotting is intentional and handles the concern.
| plt.savefig(buf, format="png") | ||
| buf.seek(0) | ||
| display(Image(buf.read())) | ||
| plt.close() |
There was a problem hiding this comment.
What happens if it's not pyodide? How do we plot RDF??
There was a problem hiding this comment.
the previous function plot_distribution from utils
There was a problem hiding this comment.
Add a comment explaining why this is needed
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
utils/plot.py (1)
89-96: Consider making the figure reference explicit.The current implementation relies on
plt.savefig()saving the "current figure" created byplot_distribution_function. If that function creates multiple figures or changes the current figure context, this might capture the wrong figure.Additionally, there's no error handling if the save or display operations fail.
🔎 Suggested improvement
Consider having
plot_distribution_functionreturn the figure object, then explicitly save that figure:+ fig = plot_distribution_function( - plot_distribution_function( rdf.bin_centers, rdf.rdf, xlabel="Distance (Å)", ylabel="g(r)", title="Radial Distribution Function (RDF)" ) if is_pyodide: import matplotlib.pyplot as plt - buf = io.BytesIO() - plt.savefig(buf, format="png") - buf.seek(0) - display(Image(buf.read())) - plt.close() + try: + buf = io.BytesIO() + fig.savefig(buf, format="png") + buf.seek(0) + display(Image(buf.read())) + finally: + plt.close(fig)This assumes
plot_distribution_functioncan be modified to return the figure. If not, the current approach should work as long as it's the only figure being created.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utils/plot.py
🧰 Additional context used
🧬 Code graph analysis (1)
utils/plot.py (1)
other/generate_gifs/utils/gif_processor.py (1)
display(197-202)
🔇 Additional comments (1)
utils/plot.py (1)
67-68: LGTM! PEP 8 formatting improvement.Adding spaces around the
+operator improves code style consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
utils/plot.py (1)
67-68: Minor: Formatting changes unrelated to PR objective.The spacing improvements around
i + 1are fine and improve readability, but these changes inplot_twisted_interface_solutionsappear unrelated to the RDF plot fix mentioned in the PR objective.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utils/plot.py
🧰 Additional context used
🧬 Code graph analysis (1)
utils/plot.py (1)
other/generate_gifs/utils/gif_processor.py (1)
display(197-202)
🔇 Additional comments (2)
utils/plot.py (2)
1-2: LGTM! Imports support the Pyodide functionality.The new imports (
io,sys,IPython.display.Image,IPython.display.display, andmatplotlib.pyplot) are all necessary for the Pyodide-aware plotting implementation and are correctly placed at the module level.Also applies to: 5-5, 10-10
76-94: Well-structured Pyodide-aware plotting logic.The implementation correctly addresses the previous review feedback by using
plt.switch_backend("Agg")(which works at runtime) instead ofmatplotlib.use(). The flow is sound:
- Pyodide path: switches to non-interactive Agg backend → creates plot → captures to BytesIO buffer → displays inline → cleans up
- Non-Pyodide path: uses default backend →
plot_distribution_functionhandles display (as confirmed in past comments)The Pyodide detection via
sys.platform == "emscripten"is the standard approach.Optional verification: Test this in both Pyodide (JupyterLite) and standard Jupyter environments to confirm:
- The RDF plot renders correctly as an inline PNG in Pyodide
- The plot displays normally in non-Pyodide environments
- No matplotlib backend warnings appear in either environment
Optional refinement (explicit figure management)
For more explicit control, you could capture the figure reference instead of relying on implicit current-figure behavior:
if is_pyodide: buf = io.BytesIO() fig = plt.gcf() # Get current figure explicitly fig.savefig(buf, format="png") buf.seek(0) display(Image(buf.read())) plt.close(fig)This makes the code slightly more robust but isn't necessary given the current implementation works correctly.
Summary by CodeRabbit
New Features
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.