Move from GaussQuadrature.jl to FastGaussQuadrature.jl
#2261
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @sriharshakandala, 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!
Summary of Changes
This pull request migrates the codebase from using the GaussQuadrature package to FastGaussQuadrature.jl. This involves updating the Project.toml file to remove the dependency on GaussQuadrature and add a dependency on FastGaussQuadrature, as well as modifying the Quadratures.jl file to use the functions from FastGaussQuadrature for calculating quadrature points and weights. The orthonormal_poly function is also commented out.
Highlights
- Dependency Update: Replaces
GaussQuadraturewithFastGaussQuadrature.jlin the project dependencies. - Code Modernization: Updates the
Quadratures.jlfile to utilizeFastGaussQuadraturefor Gauss-Legendre and Gauss-Legendre-Lobatto quadrature calculations. - Functionality Change: Comments out the
orthonormal_polyfunction and related code inQuadratures.jl.
Changelog
- Project.toml
- Removed
GaussQuadratureas a dependency. - Added
FastGaussQuadratureas a dependency. - Updated versions of
CubedSphere,DataStructures,FastBroadcast,ForwardDiff,HDF5
- Removed
- src/Quadratures/Quadratures.jl
- Replaced
import GaussQuadraturewithimport FastGaussQuadrature. - Modified
quadrature_pointsfunction forGLLto useFastGaussQuadrature.gausslobatto(line 66). - Modified
quadrature_pointsfunction forGLto useFastGaussQuadrature.gausslegendre(line 82). - Commented out the
orthonormal_polyfunction and related code (lines 225-252).
- Replaced
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
Trivia time!
What is the origin of the term 'quadrature' in mathematics?
Click here for the answer
The term 'quadrature' comes from the problem of 'squaring' a shape, i.e., finding a square with the same area as the given shape. In numerical analysis, it refers to the numerical approximation of definite integrals.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request replaces the GaussQuadrature package with FastGaussQuadrature.jl. The changes look good overall, and the code is clear. Here are a few suggestions for improvement.
Summary of Findings
- Documentation URL: Consider adding a brief explanation of why the linked mathworld.wolfram.com URL is relevant to the code.
- Uncommented Code: The commented out code block should be removed, as it's no longer used.
Merge Readiness
The code changes are well-structured and the migration from GaussQuadrature to FastGaussQuadrature.jl seems to be implemented correctly. I recommend addressing the review comments before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.
| @generated function quadrature_points(::Type{FT}, ::GLL{Nq}) where {FT, Nq} | ||
| points, weights = GaussQuadrature.legendre(FT, Nq, GaussQuadrature.both) | ||
| :($(SVector{Nq}(points)), $(SVector{Nq}(weights))) | ||
| points, weights = FastGaussQuadrature.gausslobatto(Nq) | ||
| :($(SVector{Nq, FT}(points)), $(SVector{Nq, FT}(weights))) |
There was a problem hiding this comment.
It's good that you're specifying the floating point type FT in the SVector constructor. This ensures type stability and avoids potential performance issues. However, the gausslobatto function from FastGaussQuadrature already returns points and weights of type Float64. You may want to consider converting the output of gausslobatto to FT directly to avoid potential type mismatches or unexpected behavior.
| @generated function quadrature_points(::Type{FT}, ::GL{Nq}) where {FT, Nq} | ||
| points, weights = GaussQuadrature.legendre(FT, Nq, GaussQuadrature.neither) | ||
| :($(SVector{Nq}(points)), $(SVector{Nq}(weights))) | ||
| points, weights = FastGaussQuadrature.gausslegendre(Nq) | ||
| :($(SVector{Nq, FT}(points)), $(SVector{Nq, FT}(weights))) |
There was a problem hiding this comment.
Similar to the GLL quadrature, it's good that you're specifying the floating point type FT in the SVector constructor. This ensures type stability and avoids potential performance issues. However, the gausslegendre function from FastGaussQuadrature already returns points and weights of type Float64. You may want to consider converting the output of gausslegendre to FT directly to avoid potential type mismatches or unexpected behavior.
|
@szy21 , @akshaysridhar : Please advise if we are currently using the functions |
sriharshakandala
changed the title
GaussQuadrature to FastGaussQuadrature.jlGaussQuadrature.jl to FastGaussQuadrature.jl
sriharshakandala
force-pushed
the
sk/move_to_fastgaussquadrature
branch
from
4b4c349 to
ce7e6de
Compare
|
Yes. To be specific, do we need these functions in the future or can we remove them (the functions and the |
sriharshakandala
force-pushed
the
sk/move_to_fastgaussquadrature
branch
from
ce7e6de to
f30209b
Compare
sriharshakandala
force-pushed
the
sk/move_to_fastgaussquadrature
branch
from
f30209b to
af7fbac
Compare
|
@Sbozzolo : Do we need to build the |
Good catch! We don't need them! |
`lib/ClimaCoreTempestRemap` projects from the Buildkite pipeline.
Move from using
GaussQuadraturetoFastGaussQuadrature.jl