Object versioning for DataCarrier object

[nbouziani]

Small changes in DataCarrier to know if the value of DataCarrier.dat._data has been modified (e.g. object versioning). The purpose of these changes is to keep track of the value of firedrake.Constant objects.

[wence-]

This is a good start, but not all the places in which the data in a Dat or matrix can be modified. In petsc_base.py the .vec, and vec_wo context managers also induce modification of the data. (see VecAccessMixin). For matrices you should probably update the version in assemble (but also other functions that sound like they modify data, like zero_rows/set_values...).

Additionally, the halo exchange functions (global_to_local_end/local_to_global_end) might change data state as well (the user is allowed to call these).

Finally, the big one, a par_loop also updates argument state of all arguments other than READ arguments. Probably adapt ParLoop.update_arg_data_state.

[nbouziani]

Thank you very much for you advice! I do apologize for my late reply, I have completely forgotten about this pending task! Yes, I initially pushed changes to handle op2.Global without considering op2.Dat/op2.Mat because object versioning was at that time needed for firedrake.Constant only.

I have just pushed some changes following your comment.

Regarding assemble: did you mean firedrake.assemble ? My first guess to catch the various assembly calls performed on base.Mat was to update the data version in petsc_base.Mat.assemble but I am not sure that all assembly calls go through this route. In particular, unlike firedrake.Matrix, firedrake.ImplicitMatrix does not seem to rely on base.Mat. Instead it uses the PETSc matrix object (PETSc.Mat()), and I am not sure how these two play with each other.

What do you think ?

[nbouziani]

Naive question for ParLoop: Why would we need to adapt update_arg_data_state (which is called in ParLoop.compute) whereas local_to_global_end, which is called after, will take into account the access mode to update the data version ?

[wence-]

Aside, I don't know what you mean by "finality: firedrake.Constant".

[nbouziani]

Following previous discussions, object versioning is now implemented using the PETSc state counter.

Testing failure is due to the fact that tests don't rely on the PETSc branch that makes the state counter public. All tests are passing for me locally.

[nbouziani]

Tests now rely on the appropriate petsc branch. Related PR: firedrakeproject/petsc#8

[dham]

declare this abstract.

[nbouziani]

The problem is that this requires all the subclasses to implement it and at the moment Global, Dat and MixedDat get the implementation of increment_dat_version via another route, i.e. VecAccessMixin. That’s because we don’t want to duplicate the object versioning setup in Global and AbstractDat objects so I put it in the appropriate common ancestor (VecAccessMixin).

[wence-]

I think the vec_context contextmanager on dats needs changing (it always calls self._vec.stateIncrease() but it should now not bother, because we correctly manipulate the state of things, I think).

[nbouziani]

I think the vec_context contextmanager on dats needs changing (it always calls self._vec.stateIncrease() but it should now not bother, because we correctly manipulate the state of things, I think).

Yes I think that’s right, because calling stateIncrease() was the result of modifying Vec off PETSc’s radar in places like data or zeros, which we now catch. I have removed that which caused the following test to fail:

d1 = op2.Dat(...)
with d1.vec_wo as _:
	…
# Check that d1.dat_version has increased

but I think the problem here is that the test is wrong since we don’t want to update the object version if we just use a context manager but rather if we actually changes the value of d1 inside the context manager. So I removed the test as well.

[wence-]

I think the vec_context contextmanager on dats needs changing (it always calls self._vec.stateIncrease() but it should now not bother, because we correctly manipulate the state of things, I think).

Yes I think that’s right, because calling stateIncrease() was the result of modifying Vec off PETSc’s radar in places like data or zeros, which we now catch. I have removed that which caused the following test to fail:

d1 = op2.Dat(...)
with d1.vec_wo as _:
	…
# Check that d1.dat_version has increased

but I think the problem here is that the test is wrong since we don’t want to update the object version if we just use a context manager but rather if we actually changes the value of d1 inside the context manager. So I removed the test as well.

let us check that the interaction with petsc works well and modify vec.array in the context manager. the state should then increase.