Skip to content

Fix Trefethen(Strip)General#305

Open
merreeves wants to merge 2 commits into
theochem:masterfrom
merreeves:master
Open

Fix Trefethen(Strip)General#305
merreeves wants to merge 2 commits into
theochem:masterfrom
merreeves:master

Conversation

@merreeves

@merreeves merreeves commented May 29, 2026

Copy link
Copy Markdown

TrefethenGeneral and TrefethenStripGeneral OneDGrids can take any OneD Quadrature and transform it. Some OneDGrids have extra parameters which the Trefethen transforms don't account for (e.g. TanhSinh has an additional parameter delta: float = 0.1). The added code corrects this and have tested it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for forwarding extra keyword arguments to the inner quadrature constructor inside TrefethenGeneral and TrefethenStripGeneral, so quadratures with additional parameters (e.g. TanhSinh's delta) can be used.

Changes:

  • Added **quadrature_params to TrefethenGeneral.__init__ and forwarded them to the inner quadrature(npoints, ...) call.
  • Added the same **quadrature_params pass-through to TrefethenStripGeneral.__init__.
  • Added a unit test verifying TrefethenGeneral accepts and uses extra quadrature params via TanhSinh.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/grid/onedgrid.py Adds **quadrature_params to TrefethenGeneral and TrefethenStripGeneral, forwarding to the inner quadrature; updates TrefethenGeneral docstring.
src/grid/tests/test_onedgrid.py Adds a regression test exercising TrefethenGeneral with an extra delta parameter for TanhSinh.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/grid/onedgrid.py
name = "Trefethen-Strip-General"

def __init__(self, npoints: int, quadrature, rho: float = 1.1):
def __init__(self, npoints: int, quadrature, rho: float = 1.1, **quadrature_params):
@marco-2023

Copy link
Copy Markdown
Collaborator

@merreeves and @PaulWAyers, in my opinion, the way that TrefethenGeneral is initialized (from a OneDGrid trype and the number of points ) is obscure and fragile. It would be way easier to just give a grid instance. I have a doubt, is there a specific reason why we don't treat TrefethenGeneral as a transformation?

@PaulWAyers

Copy link
Copy Markdown
Member

I don't think there is any reason not to treat (both of) the Trefethen tricks as a transformation. As I recall, the mathematical argument is pretty tightly tied to Chebychev/Fejer/Clenshaw-Curtis quadrature, but there is no real reason to not generalize.

@PaulWAyers PaulWAyers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants