Documentation #96
Documentation #96
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
src/vampyr/treebuilders/project.h
Outdated
| try { | ||
| auto arr = std::array<double, D>(); | ||
| arr.fill(111111.111); // A number which hopefully does not divide by zero | ||
| func(arr); | ||
| } catch (py::cast_error &e) { | ||
| py::print("Error: Invalid definition of analytic function"); | ||
| throw; | ||
| } |
There was a problem hiding this comment.
what's this block supposed to do?
There was a problem hiding this comment.
It catches a cast_error which kills Jupyter kernels. Want a comment there? When we try to project analytic functions.
So I try to evaluate the function in a random point, and see if it works or not.
src/vampyr/treebuilders/project.h
Outdated
| @@ -1,7 +1,8 @@ | |||
| #pragma once | |||
|
|
|||
| #include <pybind11/functional.h> | |||
| #include <typeinfo> | |||
There was a problem hiding this comment.
I can't see anything from this header being used.
bjorgve
force-pushed
the
documentation
branch
from
3012534
to
2f5836a
Compare
docs/requirements.txt
Outdated
| @@ -1,5 +1,6 @@ | |||
| myst_nb | |||
| sphinx | |||
| sphinx_rtd_theme | |||
There was a problem hiding this comment.
I don't think this is needed.
| @@ -18,6 +17,18 @@ template <int D> void project(pybind11::module &m) { | |||
| .def( | |||
| "__call__", | |||
| [](PyScalingProjector<D> &P, std::function<double(const Coord<D> &r)> func) { | |||
|
|
|||
| try { | |||
There was a problem hiding this comment.
Now I see what you want to do. I don't think it's the best strategy, as you can't be sure this test won't give you a false positive. I think what is explained here is what should be done instead: https://pybind11.readthedocs.io/en/stable/advanced/exceptions.html#handling-exceptions-from-python-in-c
The try-catch block should be around auto out = P(func);, I believe.
There was a problem hiding this comment.
Yes I did try that first, but for some reason I found it a bit challenging. I think it had to do with the return, I can have another look at it.
| P_scaling = vp.ScalingProjector(mra, 2) | ||
| P_wavelet = vp.WaveletProjector(mra, 2) | ||
|
|
||
| with pytest.raises(Exception): |
There was a problem hiding this comment.
I don't see how use f(x) = x will raise an exception here. Same in the next test.
There was a problem hiding this comment.
This syntax does not work in vampyr (probably should in 1d) and causes the kernel to crash (in Jupyter notebooks). Correct syntax is f(x) = x[0]
There was a problem hiding this comment.
Now I see it 🤦🏻 the signature is std::function<double(const Coord<D> &r)> so it expects one number out of it. It's not exactly great style to raise/catch Exception (because it's the base class of all exceptions in Python) but I think it's appropriate here, because one cannot know exactly what went wrong when calling the function.
There was a problem hiding this comment.
For the notebooks:
- To be rendered correctly in the TOC, make sure to add a title at the top, i.e. a line starting with
# Title - I suggest you put a box at the top with author-date and also the image explaining how to run. Like here: https://github.com/ENCCS/veloxchem-workshop/blob/master/content/notebooks/first-steps.ipynb (you can copy-paste from the link) The top box can be marked as "hidden" so it's not rendered in the documentation: https://jupyterbook.org/en/stable/content/metadata.html#jupyter-cell-tags
|
Okei, thanks for the feedback. I'll fix it asap. |
I think it's time to update master with some docs and fix to killed kernels when badly defined analytic functions are projected.
#95