Conversation
|
Update https://github.com/lululxvi/deepxde/blob/master/docs/demos/operator.rst so the example shows up at https://deepxde.readthedocs.io/en/latest/demos/operator.html |
Done |
echen5503
left a comment
echen5503
left a comment
There was a problem hiding this comment.
Hi, I'm relatively new here so please take my comments with a grain of salt, but I found some potential issues with the code. Reviewing to learn more right now :)
|
|
||
|
|
||
| def pde(x, y, v): | ||
| # theta''(t) = -sin(theta(t)) + u(t) |
There was a problem hiding this comment.
Nitpick: Why not use theta unicode character?
There was a problem hiding this comment.
This is inconsistent with the other examples, since unicode isn’t used for Greek letters anywhere else.
| - `Diffusion reaction equation with aligned points <https://github.com/lululxvi/deepxde/tree/master/examples/operator/diff_rec_aligned_pideeponet.py>`_ | ||
| - `Diffusion reaction equation with unaligned points <https://github.com/lululxvi/deepxde/tree/master/examples/operator/diff_rec_unaligned_pideeponet.py>`_ | ||
| - `Stokes flow with aligned points <https://github.com/lululxvi/deepxde/tree/master/examples/operator/stokes_aligned_pideeponet.py>`_ | ||
| - `Gravity pendulum with unaligned points <https://github.com/lululxvi/deepxde/tree/master/examples/operator/gravity_pendulum_unaligned_pideeponet.py>`_ |
There was a problem hiding this comment.
It may be a good idea to have some documentation for this code. See https://github.com/lululxvi/deepxde/pull/2056/changes.
It seems to be uniquely valuable here because operator learning has few examples with documentation.
| """ | ||
| Solve the forced pendulum ODE using a 4th-order Runge–Kutta method | ||
|
|
||
| theta''(t) = -sin(theta(t)) + u(t), |
There was a problem hiding this comment.
PDE expression already duplicated above
There was a problem hiding this comment.
This doesn’t seem like a significant issue to me.
| import matplotlib.pyplot as plt | ||
| import numpy as np | ||
|
|
||
| sin = dde.backend.sin |
There was a problem hiding this comment.
This renamed sin function is only used twice. It may be better to just use dde.backend.sin for clarity.
| return on_boundary and np.isclose(x[0], 0.0) | ||
|
|
||
|
|
||
| geom = dde.geometry.TimeDomain(0, 1) |
There was a problem hiding this comment.
This part is likely difficult to read for newcomers. Comments / Documentation would probably help.
| v = u_sin(eval_pts) | ||
| solved_theta = solve_pendulum(u_sin, t) | ||
| predicted_theta = model.predict((np.tile(v.T, (t.size, 1)), t.reshape(-1, 1))) | ||
|
|
There was a problem hiding this comment.
Could be helpful to add error metrics here
There was a problem hiding this comment.
I’m not sure it makes much sense to use metrics for just one example of u(t).
There was a problem hiding this comment.
Right, it's already plotted. SG
|
@echen5503 Thank you for your careful review. I have updated the parts that seemed most important to address. |
3 participants
Add a PI-DeepONet example for the gravity pendulum problem: