Remove PETScWrappers deprecations

[masterleinad]

Deprecated in #2749 and some time before #6986.

[agrayver]

I'm actually using these quite extensively, including for some rectangular structural matrices (i.e. without an associated FE space). Once this is merged, the only remaining constructor will require a SparsityPattern.

How do I replace somehing like PETScWrappers::MPI::SparseMatrix(comm, rows, columns, local_rows, local_columns, nnz_per_row) with a SparsityPattern requiring constructor?

It seems it will require constructing SparsityPattern manually before initializing the matrix.

[masterleinad]

It seems it will require constructing SparsityPattern manually before initializing the matrix.

Yes, this has been deprecated for a couple of years already without any complaints.

[agrayver]

Perhaps I miss something.

If I can create a PETSc matrix by using PETScWrappers::MPI::SparseMatrix(comm, m, n, local_rows, local_columns, nnz_per_row) (which calls legitimate MatCreateAIJ), what would I gain by using SparsityPattern instead? In fact, using SparsityPattern requires additional memory, time and code because I will have to loop through all rows/cols and add entries to a SparsityPattern followed by matrix initialization and actual assembly.

I specifically mean matrices, which are not square system matrices, but other objects.

[masterleinad]

@dealii/dealii Can someone that uses PETSc some more can chime in here?

[drwells]

I'll take a look.

[drwells]

Sorry for the delays - I promise I will get to this soon!

[drwells]

We have talked about support for this kind of operation in other places (if I recall correctly we want to support wrapping Mat and Vec without being responsible for setting up or destroying them).

@agrayver We deprecated this since we wanted to make the interface to all of our sparse matrix classes the same. Would a matrix view that owns a Mat (and lets the user set it up as they want) work for you?

[agrayver]

@drwells thanks for looking at it.

Would a matrix view that owns a Mat (and lets the user set it up as they want) work for you?

Yes, it would. Is there such a mechanism?

[agrayver]

More specifically, do you mean one could add a constructor like?

PETScWrappers::MPI::SparseMatrix(Mat& mat)

If this is ok, I could add such constructor quickly.

[kronbichler]

I agree with this and would be happy to take a follow-up PR that adds a constructor. @drwells do you agree?

[bangerth]

It's not that easy. The model right now is that the class actually owns the pointer and its destructor is responsible for destroying the object. If you pass in a Mat pointer, you need to also define who now owns the object? I have to admit that I don't particularly like the model where a class may or may not own an object.

Separately, Mat is conceptually a "pointer to base class". You'd have to ensure that it really does correspond to a sparse matrix.

[tjhei]

you need to also define who now owns the object?

I think we could modify the PETSc internal reference count using PetscObjectReference / PetscObjectDereference. The last call to MatDestroy will then delete it.

[agrayver]

I think we could modify the PETSc internal reference count using PetscObjectReference / PetscObjectDereference. The last call to MatDestroy will then delete it.

Yes, this seems a fully legit way that is used in PETSc examples, e.g.:

https://www.mcs.anl.gov/petsc/petsc-current/src/ts/tutorials/ex3.c.html

[masterleinad]

Alright. @agrayver you were saying that you could add a constructor quickly? We can then decide what we want to have in the release and what not.

[bangerth]

Huh, I had no idea that PETSc does reference counting on its objects...

[drwells]

Its not that obvious from the names (or PETSc's documentation) but @agrayver is absolutely right. PETSc does reference count:

PetscErrorCode MatDestroy(Mat *A)
{
  PetscErrorCode ierr;

  PetscFunctionBegin;
  if (!*A) PetscFunctionReturn(0);
  PetscValidHeaderSpecific(*A,MAT_CLASSID,1);
  if (--((PetscObject)(*A))->refct > 0) {*A = NULL; PetscFunctionReturn(0);}
...

I am not sure that we have enough time to do this before the release: we would also need to check that older versions of PETSc implement this logic, add tests for the new feature, etc. I believe it would be best to preserve the status quo (i.e., not merge this patch [1]) and then fix things once the release is out (in fact, I have 'update PETSc stuff' on my hackathon list).

I really like the idea of adding views to PETSc objects this way - this is much better than what I had in mind!

[1] By this patch I mean the present PR.

[bangerth]

Yes, let's not address accepting external Mat and Vec objects before the release. This requires a while of testing, I think.

[masterleinad]

For me, the question is whether we still want to merge this pull request for the release or not.

[drwells]

Shall we vote? I think we should not merge this PR yet.

[tjhei]

I think we should not merge this PR yet.

Same here, let's not break anything right before the release.

[masterleinad]

Fine. Let's just remove it from the milestone then.

[peterrum]

I just quote the above statement and adjust the milestone:

Same here, let's not break anything right before the release.

[peterrum]

Do we want to tackle this shortly after next release?

[masterleinad]

I would be happy if we could finally clean this up.

[agrayver]

Since I was the one who used these wrappers, just wanted to say that I have found an alternative way of doing things and do not need them anymore.

[masterleinad]

I merged this pull request with master to resolve merge conflicts. Apparently, we already merged most of the changes in #12288.

[masterleinad]

This should be ready now.