[Math][Doc] add a example of performing abitrary function mapping #1473
Conversation
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
Summary of ChangesHello @tdavidcl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new documentation example that illustrates the technique of arbitrary function mapping, specifically using the inverse transform sampling method. The example demonstrates how to transform a set of uniformly distributed random points to follow the distribution of a user-defined symbolic function, involving steps like numerical integration, normalization, and interpolation for practical application. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Workflow reportworkflow report corresponding to commit 51df445 Light CI is enabled. This will only run the basic tests and not the full tests. Pre-commit check reportSome failures were detected in base source checks checks. ❌ ruff-check❌ end-of-file-fixer❌ blackSuggested changesDetailed changes :diff --git a/doc/sphinx/examples/math/run_mapping.py b/doc/sphinx/examples/math/run_mapping.py
index 84f72429..7f3e730c 100644
--- a/doc/sphinx/examples/math/run_mapping.py
+++ b/doc/sphinx/examples/math/run_mapping.py
@@ -9,22 +9,21 @@ This example shows how to use the unit vector generator
import matplotlib.pyplot as plt # plots
import numpy as np # sqrt & arctan2
-
-import shamrock
import sympy as sp
-from scipy.special import erfinv, erf
+from scipy.special import erf, erfinv
+import shamrock
-#random set of points between 0 and 1
+# random set of points between 0 and 1
np.random.seed(111)
points = np.random.rand(1000)[:]
-range_start = (0,3)
-range_end = (0,1) # must be between 0 and 1 because of the normalization
+range_start = (0, 3)
+range_end = (0, 1) # must be between 0 and 1 because of the normalization
-#define the function exp(-x^2) using sympy
-x = sp.symbols('x')
-f = sp.Abs(sp.sin(-x**2))
+# define the function exp(-x^2) using sympy
+x = sp.symbols("x")
+f = sp.Abs(sp.sin(-(x**2)))
# Numerical integration of f over range_start
primitive = []
@@ -70,9 +69,10 @@ plt.xlabel("x")
plt.ylabel("finv(x)")
plt.title("finv(x) = inverse(primitive(x))")
-#interpolate primitive using scipy.interpolate.interp1d
+# interpolate primitive using scipy.interpolate.interp1d
from scipy.interpolate import interp1d
-mapping_interp = interp1d(primitive, primitive_x, kind='linear')
+
+mapping_interp = interp1d(primitive, primitive_x, kind="linear")
points_mapped = [mapping_interp(point) for point in points]
@@ -96,4 +96,4 @@ plt.plot(r, [f.subs(x, x_val).evalf() for x_val in r])
plt.xlabel("$r$")
plt.ylabel("$f(r)$")
-plt.show()
\ No newline at end of file
+plt.show()
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new example script for performing arbitrary function mapping. The script has several issues, including incorrect documentation and comments, unused imports, and inefficient, non-idiomatic Python code. I've provided suggestions to correct the documentation, clean up the code, and improve performance and readability by using more appropriate functions from numpy and scipy.
| Shamrock 3D unit vector generator | ||
| ======================================= | ||
|
|
||
| This example shows how to use the unit vector generator |
There was a problem hiding this comment.
The docstring appears to be copied from another example and is not relevant to this script. It describes a "3D unit vector generator," whereas this example demonstrates arbitrary function mapping. Updating the docstring will make the example's purpose clear to users.
Shamrock arbitrary function mapping
===================================
This example shows how to map a set of uniformly distributed random points
to a distribution following an arbitrary function.| range_start = (0,3) | ||
| range_end = (0,1) # must be between 0 and 1 because of the normalization | ||
|
|
||
| #define the function exp(-x^2) using sympy |
There was a problem hiding this comment.
The comment incorrectly describes the function as exp(-x^2). The actual function being defined is |sin(-x^2)|. This should be corrected to avoid confusion.
# define the function f(x) = |sin(-x^2)| using sympy| # Numerical integration of f over range_start | ||
| primitive = [] | ||
| primitive_x = [] | ||
| accum = 0 | ||
| dx = (range_start[1] - range_start[0]) / 100 | ||
| for x_val in np.linspace(range_start[0], range_start[1], 100): | ||
| primitive.append(accum) | ||
| primitive_x.append(x_val) | ||
| accum += f.subs(x, x_val).evalf() * dx | ||
|
|
||
| primitive = np.array(primitive) | ||
| primitive_x = np.array(primitive_x) |
There was a problem hiding this comment.
The numerical integration is implemented with a Python for loop, which is inefficient and less accurate than available library functions. Using sympy.lambdify for faster function evaluation and scipy.integrate.cumulative_trapezoid for integration will make the code more performant, idiomatic, and accurate. Also, the dx calculation is slightly off for the number of points generated by linspace.
# Lambdify the sympy function for faster evaluation with numpy
f_numpy = sp.lambdify(x, f, 'numpy')
# Numerical integration of f over range_start
# Use more points for a better approximation of the integral
num_points = 1001
primitive_x = np.linspace(range_start[0], range_start[1], num_points)
f_vals = f_numpy(primitive_x)
primitive = cumulative_trapezoid(f_vals, primitive_x, initial=0)| f_plot = [f.subs(x, x_val).evalf() for x_val in x_plot] | ||
| plt.plot(x_plot, f_plot) | ||
| plt.xlabel("x") | ||
| plt.ylabel("f(x)") | ||
| plt.title("f(x) = exp(-x^2)") |
There was a problem hiding this comment.
The plot title is incorrect and evaluation of the function inside a list comprehension is inefficient. The title should reflect the actual function being plotted, |sin(-x^2)|. For performance, it's better to evaluate the function on the whole x_plot array at once, especially after lambdifying the sympy expression.
f_plot = f_numpy(x_plot)
plt.plot(x_plot, f_plot)
plt.xlabel("x")
plt.ylabel("f(x)")
plt.title("f(x) = |sin(-x^2)|")| from scipy.interpolate import interp1d | ||
| mapping_interp = interp1d(primitive, primitive_x, kind='linear') | ||
|
|
||
| points_mapped = [mapping_interp(point) for point in points] |
There was a problem hiding this comment.
Using a list comprehension here is inefficient. The interp1d object can directly accept a NumPy array, which is much faster. Vectorizing this operation is more idiomatic and performant.
points_mapped = mapping_interp(points)| import shamrock | ||
| import sympy as sp | ||
| from scipy.special import erfinv, erf |
There was a problem hiding this comment.
The modules shamrock, erfinv, and erf are imported but are not used anywhere in the script. It's good practice to remove unused imports to keep the code clean and avoid confusion.
import sympy as sp| print(f"primitive = {primitive}") | ||
| print(f"primitive_x = {primitive_x}") |
There was a problem hiding this comment.
These print statements output large arrays to the console, which is generally not desired in gallery examples. Consider removing them to keep the output clean.
| plt.title("finv(x) = inverse(primitive(x))") | ||
|
|
||
| #interpolate primitive using scipy.interpolate.interp1d | ||
| from scipy.interpolate import interp1d |
There was a problem hiding this comment.
Imports should be placed at the top of the file, following PEP 8 conventions. This makes it easier to see all dependencies at a glance. Please move this import to the top with the others.
| print(f"points = {points}") | ||
| print(f"points_mapped = {points_mapped}") |
There was a problem hiding this comment.
These print statements output large arrays to the console. For a gallery example, this is usually not necessary and clutters the output. It's better to remove them.
|
|
||
| plt.figure() | ||
| hist_r, bins_r = np.histogram(points, bins=101, density=True, range=range_end) | ||
| r = np.linspace(bins_r[0], bins_r[-1], 101) |
There was a problem hiding this comment.
The variable r is defined here but it is not used in the subsequent plotting code for the first histogram. It should be removed to avoid confusion.
1 participant
No description provided.