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

Adjust API to be more consistent #2650

Merged
merged 6 commits into from
May 10, 2023
Merged

Conversation

chrisrichardson
Copy link
Contributor

@chrisrichardson chrisrichardson commented May 8, 2023

  • use squared_norm() for both Vector and MatrixCSR
  • Use InsertMode not ScatterMode
  • Use finalize for both MatrixCSR and SparsityPattern
  • index_map() not map() for Vector

If any others seem obvious, we can add to this branch.

@chrisrichardson
Copy link
Contributor Author

Now also contains a bugfix for MatrixCSR::finalize which was leading to random results

std::vector<int> size_recv(src.size());
size_recv.reserve(1);
std::vector<int> size_recv;
size_recv.reserve(1); // ensure data is not a nullptr
Copy link
Member

Choose a reason for hiding this comment

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

The order of this line and the one below should be switched.

// ScatterMode types
enum class PyScatterMode
// InsertMode types
enum class PyInsertMode
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is clearer. What's being controlled is what happens during a scatter. Moreover, the previous naming had a consistency with PETSc, see https://petsc.org/release/manualpages/Vec/VecGhostUpdateBegin/.

The name change could be confused with switching between insert and set during assembly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was inconsistent with PETSc before, and is more inline now: PETSc has a scattermode (forward or reverse).

Copy link
Member

Choose a reason for hiding this comment

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

I think it was inconsistent with PETSc before, and is more inline now: PETSc has a scattermode (forward or reverse).

But the change in this PR is PyScatterMode -> PyInsertMode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because PyScatterMode was add or insert - it basically was the "Insert Mode" just wrongly named.

@IgorBaratta
Copy link
Member

assemble not finalize for MatrixCSR (similar to PETSc and we also use assemble for SparsityPattern)

I still prefer finalize over assemble. Or maybe a more descriptive name like scatter_values()?

@chrisrichardson
Copy link
Contributor Author

assemble not finalize for MatrixCSR (similar to PETSc and we also use assemble for SparsityPattern)

I still prefer finalize over assemble. Or maybe a more descriptive name like scatter_values()?

Yes, I think so too. I can change it back, unless anyone has a better idea - I'd like to change SparsityPattern to also use finalize() in which case...

@garth-wells
Copy link
Member

assemble not finalize for MatrixCSR (similar to PETSc and we also use assemble for SparsityPattern)

I still prefer finalize over assemble. Or maybe a more descriptive name like scatter_values()?

Yes, I think so too. I can change it back, unless anyone has a better idea - I'd like to change SparsityPattern to also use finalize() in which case...

Not crazy about finalize for the matrix :). Sounds as if it can't be re-used. Happy with it for SparsityPattern.

@chrisrichardson
Copy link
Contributor Author

In which case, the other alternative is some variant of scatter_foo*. Basically this operation is the equivalent operation to scatter_reverse that we do for Vector, except for matrix rows.
Could also be gather_rows() ...

@chrisrichardson chrisrichardson added this pull request to the merge queue May 10, 2023
Merged via the queue into main with commit 19f988a May 10, 2023
15 checks passed
@chrisrichardson chrisrichardson deleted the chris/naming-consistency branch May 10, 2023 11:16
jorgensd added a commit to jorgensd/dolfinx_ipcs that referenced this pull request May 16, 2023
jorgensd added a commit to ComputationalPhysiology/oasisx that referenced this pull request May 16, 2023
jorgensd added a commit to jorgensd/dolfinx_ipcs that referenced this pull request May 16, 2023
jorgensd added a commit to jorgensd/dolfinx_mpc that referenced this pull request May 16, 2023
jorgensd added a commit to ComputationalPhysiology/oasisx that referenced this pull request May 16, 2023
* ScatterMode->InsertMode, ref: FEniCS/dolfinx#2650

* Fix sp

* fix notebook
jorgensd added a commit to jorgensd/dolfinx_mpc that referenced this pull request May 17, 2023
* assemble()->finalize() for sparsity pattern

* Fix demo, ref FEniCS/dolfinx#2650
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.

None yet

3 participants