Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PartialDerivativeCoefficients representation to lindiffops #33

Merged
merged 19 commits into from
Jun 23, 2023

Conversation

timweiland
Copy link
Collaborator

@timweiland timweiland commented Jun 19, 2023

Each linear differential operator can be represented as a sum of partial derivatives, and this PR adds this representation to every lindiffop.

EDIT: Rework done

Copy link
Owner

@marvinpfoertner marvinpfoertner left a comment

Choose a reason for hiding this comment

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

Nice work! 🙏🏻 Just some minor comments

src/linpde_gp/linfuncops/diffops/_coefficients.py Outdated Show resolved Hide resolved
src/linpde_gp/linfuncops/diffops/_coefficients.py Outdated Show resolved Hide resolved
Copy link
Owner

@marvinpfoertner marvinpfoertner left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this 🙏🏻

One last comment: I think it would make sense to realize the multi-indices as a NumPy array with shape input_domain_shape and dtype np.int_. Let's discuss this offline.

src/linpde_gp/linfuncops/diffops/_coefficients.py Outdated Show resolved Hide resolved
src/linpde_gp/linfuncops/diffops/_coefficients.py Outdated Show resolved Hide resolved
src/linpde_gp/linfuncops/diffops/_coefficients.py Outdated Show resolved Hide resolved
src/linpde_gp/linfuncops/diffops/_coefficients.py Outdated Show resolved Hide resolved
src/linpde_gp/linfuncops/diffops/_laplacian.py Outdated Show resolved Hide resolved
src/linpde_gp/linfuncops/diffops/_coefficients.py Outdated Show resolved Hide resolved
src/linpde_gp/linfuncops/diffops/_coefficients.py Outdated Show resolved Hide resolved
src/linpde_gp/linfuncops/diffops/_coefficients.py Outdated Show resolved Hide resolved
Co-authored-by: Marvin Pförtner <[email protected]>
@timweiland
Copy link
Collaborator Author

Turns out NumPy arrays are not hashable :D
Do we want to subclass np.array and provide a custom implementation for __hash__ or do we just keep the current implementation @marvinpfoertner ?
Subclassing np.array is pretty fragile from my experience so I would personally prefer the latter

@timweiland
Copy link
Collaborator Author

Added a new MultiIndex class as discussed offline.

Copy link
Owner

@marvinpfoertner marvinpfoertner left a comment

Choose a reason for hiding this comment

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

Beautiful! 🙏🏻

src/linpde_gp/linfuncops/diffops/_coefficients.py Outdated Show resolved Hide resolved
src/linpde_gp/linfuncops/diffops/_coefficients.py Outdated Show resolved Hide resolved
src/linpde_gp/linfuncops/diffops/_coefficients.py Outdated Show resolved Hide resolved
@timweiland timweiland merged commit 9e49f25 into marvinpfoertner:main Jun 23, 2023
6 checks passed
@timweiland timweiland deleted the diffop-multi-index-dict branch June 23, 2023 23:11
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.

2 participants