Core PETSc objects/functions. #3

ZoeLeibowitz · 2023-12-07T11:03:30Z

No description provided.

ZoeLeibowitz · 2023-12-07T11:51:07Z

I rebased on top of Fabio’s branch ‘sycl-init’ hence all of the other file changes. Please review these files:

types/petsc.py
tests/test_petsc.py

I also added the function ‘dtype_to_petsctype’ inside tools/dtypes_lowering.py

Also, can someone help me understand why two of the pytests are failing?

EdCaunt · 2023-12-07T13:12:01Z

devito/types/petsc.py

+
+    def __init_finalize__(self, *args, **kwargs):
+
+        super(PETScFunction, self).__init_finalize__(*args, **kwargs)


Can just be super().__init_finalize__(*args, **kwargs)

EdCaunt · 2023-12-07T13:13:25Z

devito/types/petsc.py

+
+class PETScFunction(AbstractFunction):
+    """
+    PETScFunctions.


Might want a more fleshed out docstring in due course

EdCaunt · 2023-12-07T13:14:07Z

devito/types/petsc.py

+        self._is_const = kwargs.get('is_const', False)
+
+    @classmethod
+    def __dtype_setup__(cls, **kwargs):


cls is never used and can be dropped from the args

Unless this gets called in the super in which case it might need to stay

it must stay

why do u need to override this method?

Otherwise the dtype for each PETScFunction is None and I use the dtype inside the new function dtype_to_petsctype. I think this works for PETScFunctions because the petsctype will actually depend on the dtype of the object (e.g Function) provided by the user.

EdCaunt · 2023-12-07T13:17:13Z

devito/types/petsc.py

+    def _C_ctype(self):
+        petsc_type = dtype_to_petsctype(self.dtype)
+        modifier = '*' * len(self.dimensions)
+        customtype = CustomDtype(petsc_type, modifier=modifier)


You could just return this line

EdCaunt · 2023-12-07T13:20:10Z

tests/test_petsc.py

+    grid = Grid((2, 2))
+    x, y = grid.dimensions
+
+    ptr0 = PETScFunction(name='ptr0', dtype=np.float32, dimensions=grid.dimensions,


Would it be worth checking all the datatypes here, so np.float64, np.int64 etc?

FabioLuporini · 2023-12-19T09:04:11Z

devito/types/petsc.py

+        self._is_const = kwargs.get('is_const', False)
+
+    @classmethod
+    def __dtype_setup__(cls, **kwargs):


it must stay

FabioLuporini · 2023-12-19T09:04:28Z

devito/types/petsc.py

+            return np.float32
+
+    @classmethod
+    def __indices_setup__(cls, *args, **kwargs):


why do u need to override this method?

Otherwise the object indices are set as empty tuples because I'm inheriting from AbstractFunction.

So we only want part of DiscreteFunction but not all of it? WHich part of inheriting directly from DiscreteFunction creates issues?

FabioLuporini · 2023-12-19T09:04:36Z

devito/types/petsc.py

+        self._is_const = kwargs.get('is_const', False)
+
+    @classmethod
+    def __dtype_setup__(cls, **kwargs):


why do u need to override this method?

FabioLuporini · 2023-12-19T09:04:44Z

devito/types/petsc.py

+        return tuple(dimensions), tuple(dimensions)
+
+    @property
+    def dimensions(self):


why do u need to override this property?

Oh yes, this one should not be here

devito/types/petsc.py

mloubout · 2023-12-19T13:46:02Z

devito/tools/dtypes_lowering.py

+        np.float32: 'PetscScalar',
+        np.int64: 'PetscInt',
+        np.float64: 'PetscScalar'
+    }[dtype]


Petsc use the same type for single and double precision?

I believe PetscInt just represents an integer. Whether it is 32-bit or 64-bit depends on how you configured PETSc.

I meant the float64 vs float32

They use the same type for single and double precision but its representation depends on how the code was configured. https://petsc.org/release/manualpages/Sys/PetscScalar/

So you can't have both single and double precision in the same code?

No, as far as I'm aware PETSc doesn't support mixed precision

ZoeLeibowitz · 2023-12-20T22:18:50Z

I can achieve what I need for PETScFunctions by inheriting directly from ArrayBasic instead of Array (see types/petsc.py). However, I'm not sure if my code is a little hacky or not because it involves overriding the _C_name property. More specifically, if I do not override this property, creating this:p = PETScFunction(name='p', dimensions=.., dtype=..)generates the following TWO lines in the ccode:

PetscScalar**restrict p_vec;
float (*restrict p)[y_size] = (float (*)[y_size]) p_vec;

but if I override the _C_name property like this:

 @property
 def _C_name(self):
     return self.name

then creating the same PETScFunction as above will generate the following single line in the ccode (which is what I want):
PetscScalar**restrict p;
Does this seem ok or absolutely not? @FabioLuporini @mloubout

FabioLuporini · 2023-12-21T08:43:04Z

it's hacky but because we might have a technical issue in devito, which you're relying upon. So I don't have a direct answer here...

But I do have a question 😂

If you created:

class Wrapper(LocalObject, AbstractFunction):

The former to allow custom types, the latter to allow indexing; how simpler/messy would the implementation become?

If I were to design the types hierarchy now, I would maybe devise a series of base classes, each of these base classes providing a specific set of functionalities. For example ( I would actually need different names...):

LocalObject enables custom dtypes + other stuff (_C_init etc etc)
AbstractFunction enabling array notation (via shape, dimensions, etc...)

Or, alternatively (I guess I'm just thinking out aloud here), I would have a common base class that in theory can do anything, and then it's up to the subclasses to specifiy what is actually needed

So this is not really answering your question, I know. The truth is that I don't have an answer here about how to do things properly, and this is just food for though. I would very welcome a re-design and or improvements that help achieving what you need

Sorry if not helpful this time 😄

ZoeLeibowitz · 2023-12-21T12:54:46Z

it's hacky but because we might have a technical issue in devito, which you're relying upon. So I don't have a direct answer here...

But I do have a question 😂

If you created:
class Wrapper(LocalObject, AbstractFunction):
The former to allow custom types, the latter to allow indexing; how simpler/messy would the implementation become?

If I were to design the types hierarchy now, I would maybe devise a series of base classes, each of these base classes providing a specific set of functionalities. For example ( I would actually need different names...):

LocalObject enables custom dtypes + other stuff (_C_init etc etc)

AbstractFunction enabling array notation (via shape, dimensions, etc...)

Or, alternatively (I guess I'm just thinking out aloud here), I would have a common base class that in theory can do anything, and then it's up to the subclasses to specifiy what is actually needed

So this is not really answering your question, I know. The truth is that I don't have an answer here about how to do things properly, and this is just food for though. I would very welcome a re-design and or improvements that help achieving what you need

Sorry if not helpful this time 😄

I have just spent some time trying to inherit from both classes but I was having various problems. Whilst I was debugging, I came across this: https://github.com/devitocodes/devito/blob/3126fb0e7960094360ccb1b21abe577e0804f017/devito/passes/iet/definitions.py#L392C43-L392C43
which perhaps suggests that overriding the _C_name property is ok in this case? It just means that it will not generate the cast. If this is ok, then I think my code inheriting directly from ArrayBasic works fine (in a similar way to how ArrayObject works). If I eventually want/need the _C_init and _C_free functionality, I can add them to the class and edit the place_definitions function inside iet/definitions.py, similarly to how LocalObject works (I just tried this). What do you think about this?

FabioLuporini · 2023-12-21T17:06:51Z

yes I think it makes sense

we could also think of moving all of that LocalObject logic (_C_init etc) into a mixin class, but that's a secondary thought at this point

mloubout · 2023-12-22T17:59:52Z

devito/types/petsc.py

+    dtype = CustomDtype('KSPConvergedReason')
+
+
+class PETScFunction(ArrayBasic):


PETScArray since it's and Array now

mloubout · 2023-12-22T18:00:57Z

devito/passes/iet/definitions.py

@@ -219,6 +219,14 @@ def _alloc_object_array_on_low_lat_mem(self, site, obj, storage):

        storage.update(obj, site, allocs=decl)

+    def _alloc_petsc_array_on_low_lat_mem(self, site, obj, storage):


Looks exactly the same as _alloc_object_array_on_low_lat_mem robably only needs
elif i.is_ObjectArray or i.is_PETScArray: below

mloubout · 2024-01-04T17:29:35Z

devito/passes/iet/definitions.py

@@ -219,6 +219,17 @@ def _alloc_object_array_on_low_lat_mem(self, site, obj, storage):

        storage.update(obj, site, allocs=decl)

+    def _alloc_special_type_on_low_lat_mem(self, site, obj, storage):


That's still the exact same one as _alloc_object_array_on_low_lat_mem why does it needs a separate one

mloubout · 2024-01-04T17:30:45Z

devito/tools/dtypes_lowering.py

+        np.float32: 'PetscScalar',
+        np.int64: 'PetscInt',
+        np.float64: 'PetscScalar'
+    }[dtype]


So you can't have both single and double precision in the same code?

FabioLuporini · 2024-01-05T08:14:17Z

devito/types/array.py

+    __rkwargs__ = AbstractFunction.__rkwargs__ + ('is_const',)
+
+    def __init_finalize__(self, *args, **kwargs):
+


unneeded blank line

FabioLuporini · 2024-01-05T08:14:40Z

devito/types/petsc.py

+    def __dtype_setup__(cls, **kwargs):
+        return kwargs.get('dtype', np.float32)
+
+    @property


could be a cached_property

FabioLuporini · 2024-01-05T08:15:27Z

devito/types/petsc.py

+
+    @property
+    def _C_ctype(self):
+        petsc_type = dtype_to_petsctype(self.dtype)


nitpicking:

PETSc is so special that I would maybe put dtype_to_petsctype in here rather than in tools/

…sc_functions

… PETScArray

…he else statement

ZoeLeibowitz assigned FabioLuporini, mloubout and EdCaunt Dec 7, 2023

EdCaunt reviewed Dec 7, 2023

View reviewed changes

ZoeLeibowitz force-pushed the core_petsc branch from f8f52f6 to 7e9a5cd Compare December 13, 2023 09:23

FabioLuporini reviewed Dec 19, 2023

View reviewed changes

ZoeLeibowitz force-pushed the core_petsc branch from 7e9a5cd to 2222435 Compare December 19, 2023 13:39

mloubout reviewed Dec 19, 2023

View reviewed changes

ZoeLeibowitz force-pushed the core_petsc branch from c753526 to 92bf3bc Compare December 20, 2023 21:05

ZoeLeibowitz force-pushed the core_petsc branch from 3b718b4 to 4db870e Compare December 22, 2023 14:37

ZoeLeibowitz requested review from EdCaunt, FabioLuporini and mloubout December 22, 2023 17:56

mloubout reviewed Dec 22, 2023

View reviewed changes

ZoeLeibowitz force-pushed the core_petsc branch from e6ce3be to 47e92a9 Compare January 2, 2024 11:31

mloubout reviewed Jan 4, 2024

View reviewed changes

FabioLuporini reviewed Jan 5, 2024

View reviewed changes

FabioLuporini and others added 5 commits January 8, 2024 10:02

misc: Split Dockerfile.cpu into .cpu and .intel

5918309

try petscfunc from abstractfunc

237a228

latest.

d542865

remove data allignment

7309ef5

new petsc types

e597d4d

ZoeLeibowitz added 18 commits January 8, 2024 10:02

add const func in petscfunction

c99ea82

dsl: Simplify PETScFunction class and add diff dtypes inside test_pet…

76a8e66

…sc_functions

dsl: Inherit from DiscreteFunction for PETScFunction instead?'

1b33b0a

compiler: Switch PETScFunction to inherit from Array

12c2b8d

compiler: Edit PETScFunction to inherit from ArrayBasic

cbb4c77

misc: Fix pytest for test_petsc.py

9253747

compiler: Edit _C_ctype property of PETScFunction to utilise CustomDtype

af2e479

compiler: Edit _C_ctype property of PETScFunction

8272387

misc: Update file due to incorrect rebase

83d8143

compiler: Change name to PETScArray from PETScFunction

5f71310

compiler: Add intermediate class between PETScArray and ArrayBasic

d4f0a6a

compiler: Remove PETSc specific checks in place_definitions

5e42630

compiler: Add const functionality to ArrayBasic

5478d2e

compiler: Remove PETSc specific check from place_definitions and edit…

943f251

… PETScArray

compiler: Remove is_PETScArray

d222274

compiler: Remove elif is_ObjectArray and use alloc_object_.. inside t…

55154a8

…he else statement

misc: Delete empty file

6654c1d

misc: Move dtype_to_petsctype to petsc.py and clean up blank lines

6f375f8

ZoeLeibowitz force-pushed the core_petsc branch from 3a7fb07 to 6f375f8 Compare January 8, 2024 10:13

misc: Clean up blank line

e2e5afe

ZoeLeibowitz merged commit 7ce484e into master Jan 8, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core PETSc objects/functions. #3

Core PETSc objects/functions. #3

ZoeLeibowitz commented Dec 7, 2023

ZoeLeibowitz commented Dec 7, 2023

EdCaunt Dec 7, 2023

EdCaunt Dec 7, 2023

EdCaunt Dec 7, 2023

EdCaunt Dec 7, 2023

FabioLuporini Dec 19, 2023

FabioLuporini Dec 19, 2023

ZoeLeibowitz Dec 19, 2023

EdCaunt Dec 7, 2023

EdCaunt Dec 7, 2023

FabioLuporini Dec 19, 2023

FabioLuporini Dec 19, 2023

ZoeLeibowitz Dec 19, 2023

mloubout Dec 19, 2023

FabioLuporini Dec 19, 2023

FabioLuporini Dec 19, 2023

ZoeLeibowitz Dec 19, 2023

mloubout Dec 19, 2023

ZoeLeibowitz Dec 19, 2023

mloubout Dec 22, 2023

ZoeLeibowitz Jan 2, 2024

mloubout Jan 4, 2024

ZoeLeibowitz Jan 6, 2024

ZoeLeibowitz commented Dec 20, 2023

FabioLuporini commented Dec 21, 2023 •

edited

Loading

ZoeLeibowitz commented Dec 21, 2023

FabioLuporini commented Dec 21, 2023

mloubout Dec 22, 2023

mloubout Dec 22, 2023

mloubout Jan 4, 2024

mloubout Jan 4, 2024

FabioLuporini Jan 5, 2024

FabioLuporini Jan 5, 2024

FabioLuporini Jan 5, 2024


		def __init_finalize__(self, args, *kwargs):

		super(PETScFunction, self).__init_finalize__(args, *kwargs)

		dtype = CustomDtype('KSPConvergedReason')


		class PETScFunction(ArrayBasic):

		@@ -219,6 +219,14 @@ def _alloc_object_array_on_low_lat_mem(self, site, obj, storage):

		storage.update(obj, site, allocs=decl)

		def _alloc_petsc_array_on_low_lat_mem(self, site, obj, storage):

		@@ -219,6 +219,17 @@ def _alloc_object_array_on_low_lat_mem(self, site, obj, storage):

		storage.update(obj, site, allocs=decl)

		def _alloc_special_type_on_low_lat_mem(self, site, obj, storage):

		__rkwargs__ = AbstractFunction.__rkwargs__ + ('is_const',)

		def __init_finalize__(self, args, *kwargs):

Core PETSc objects/functions. #3

Core PETSc objects/functions. #3

Conversation

ZoeLeibowitz commented Dec 7, 2023

ZoeLeibowitz commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZoeLeibowitz commented Dec 20, 2023

FabioLuporini commented Dec 21, 2023 • edited Loading

ZoeLeibowitz commented Dec 21, 2023

FabioLuporini commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini commented Dec 21, 2023 •

edited

Loading