Add GaussianQuadrature2D - #476
Conversation
jacklovell
left a comment
jacklovell
left a comment
There was a problem hiding this comment.
Very nice, only minor comments from me. Some of the (code comment related) comments apply equally to the existing GaussianQuadrature class on which this is based, so if you can add some clarifying code comments there too that would be nice but not a deal breaker.
| - :math:`x_{\mathrm{lower}}`: Lower limit of integration for the x-dimension. | ||
| - :math:`x_{\mathrm{upper}}`: Upper limit of integration for the x-dimension. | ||
| - :math:`y_{\mathrm{lower}}(x)`: Lower limit of integration for the y-dimension, a function of :math:`x`. | ||
| - :math:`y_{\mathrm{upper}}(x)`: Upper limit of integration for the y-dimension, a function of :math:`x`. |
There was a problem hiding this comment.
Either double-escape \mathrm or (preferably) use a raw string for the docstring (r''') and go with single backslashes for all the Latex code.
There was a problem hiding this comment.
made the docstring a raw string
There was a problem hiding this comment.
You'll also need to un-escape the backslashes for TeX macros elsewhere in the docstring now that it is a raw string: a single backslash is needed everywhere otherwise the maths isn't rendered correctly when building the HTML docs.
There should also be a blank line after the .. math:: block and before where:, otherwise Sphinx complains with WARNING: Explicit markup ends without a blank line; unexpected unindent. [docutils]
./dev/build_docs.sh from the root directory of the repo is good for locally checking how the docstrings render. You'll need the sphinx, sphinx-tabs and sphinx-rtd-theme Python packages installed.
There was a problem hiding this comment.
Thanks, I hope that now it is ok.
To keep consistent naming with function frameword and quadrature2D integrator. GaussianQuadrature class marked as deprecated.
|
Thanks for the review @jacklovell! I implemented changes to take into account the suggestions. I refactored the |
Docstring was improved, explanation of order increase added. The evaluate method was refactored to improve efficiency Redundant methods for testing were removed
Multiple values and combinations of max_orders and min_orders are now tested.
| Integrators | ||
| ------------- | ||
|
|
||
| .. autoclass:: cherab.core.math.integrators.integrators1d.GaussianQuadrature |
There was a problem hiding this comment.
Should be GaussianQuadrature1D
There was a problem hiding this comment.
Good catch, I found few more GaussianQuadrature imports/usage and replaced it. I hope that it is all now.
|
Great, thanks. 2 final (minor) unresolved comments relating to the documentation and then it'll be good to merge. |
|
I hope I've addressed all the outstanding comments and problems, so this one is ready for another go. |
2 participants
Adds
GaussianQuadrature2Dclass mentioned in #475.Adds also integrators (1D, 2D) to our documentation.
This PR will be marked as draft until PR #473 is merged.