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

Refactor la.Vector.norm #3108

Merged
merged 13 commits into from
Mar 26, 2024
Merged

Refactor la.Vector.norm #3108

merged 13 commits into from
Mar 26, 2024

Conversation

nate-sime
Copy link
Contributor

@nate-sime nate-sime commented Mar 20, 2024

Closes #3103.

Refactor la.Vector.norm into a function to reflect the C++ layer.
Add implementation of integer types for la.Vector.
Redesign the la.Vector tests to parametrize over dtype as well as xfail on unsupported (or not yet implemented) norm types.

@nate-sime nate-sime marked this pull request as ready for review March 20, 2024 16:57
Copy link
Sponsor Member

@jorgensd jorgensd left a comment

Choose a reason for hiding this comment

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

Looks good to me, even if it means that we change the API:P

Only request is to add support for l1 and l-inf norms for integers, as they are supported. "l2" is clearly not supported.

I guess if @garth-wells or @IgorBaratta or @chrisrichardson has any opinions on this please chip in:)

@nate-sime
Copy link
Contributor Author

Regarding supporting norms of vectors of integral types, I'm having a little trouble coming up with a neat way to deduce the underlying type of a vector of complex whilst also handling those integral types. The outline for this is clearly in dolfinx::scalar_value_type_t; however, this would either require another similar definition with carefully chosen name, or expansion of concept scalar which I don't believe fits in with the scientific computing philosophy. Perhaps this could be approached in a future pull request?

@nate-sime nate-sime added this pull request to the merge queue Mar 22, 2024
@garth-wells garth-wells removed this pull request from the merge queue due to a manual request Mar 22, 2024
Copy link
Member

@garth-wells garth-wells left a comment

Choose a reason for hiding this comment

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

I have some comments - will post tomorrow.

Copy link
Member

@garth-wells garth-wells left a comment

Choose a reason for hiding this comment

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

Looks good - just a micro change suggestion.

cpp/dolfinx/la/Vector.h Outdated Show resolved Hide resolved
python/dolfinx/wrappers/la.cpp Outdated Show resolved Hide resolved
python/dolfinx/wrappers/la.cpp Show resolved Hide resolved
@nate-sime
Copy link
Contributor Author

I think this one's ready to go now if we can put it in the merge queue, @garth-wells ?

@garth-wells garth-wells added this pull request to the merge queue Mar 26, 2024
Merged via the queue into main with commit 6189a7e Mar 26, 2024
19 checks passed
@garth-wells garth-wells deleted the nate/vector-norm-refactor branch March 26, 2024 15:37
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.

Refactor Python wrapper method la.Vector.norm into function
4 participants