Feedback on `demo_poisson.py`

Hi,

I am a FEniCS user and I am currently exploring the FEniCSx demos in order to start learning the new syntax for FEniCSx. I started with the demo_possion.py, and I am sharing my thoughts here in order to express what I think could be be done in a different way, and what I think is currently missing. If this is not the correct format to provide such feedback please let me know. I would also like to say that many of the things I highlight here expresses my opinions which is definitely not necessarily the best for FEniCSx.

I would be happy to help out with a pull request if you think it is appropriate.

Imports

Do you have any guidelines in terms of import order? I think it is worth consider to have a consistent way of importing modules, such as first python std library, then third party and then dolfinx. Futhermore, within each group it should be alphabetical, and you can use a tool such as isort or reorder-python-imports to do this automatically (or as a pre-commit hook).
It should be some standard way in how you import modules and functions. For example, I think doing both
```
from dolfinx import fem
```
and
```
from dolfinx.fem import FunctionSpace
```
is a bit redundant. Why not just use fem.FunctionSpace? Similarly I think it would by good to just use e.g
```
import ufl
ufl.grad
```
I think this also aligns well with how people use other modules, such as numpy. This way there is no ambiguity when you use np.exp and ufl.exp for example.

Positional vs Keyword arguments

I don't know if you want to stick to using positional arguments. In my opinion it is more clear if one uses keyword arguments for most things. As an example, I think it is better to write

mesh = create_rectangle(
    comm=MPI.COMM_WORLD, 
    points=((0.0, 0.0), (2.0, 1.0)), 
    n=(32, 16), 
    cell_type=CellType.triangle
)

than

mesh = create_rectangle(
    MPI.COMM_WORLD,
    ((0.0, 0.0), (2.0, 1.0)),
    (32, 16),
    CellType.triangle
)

FunctionSpace

I think it is great that you have started to use type hint in dolfinx. This will make it much better to work with in an IDE.

I think the FunctionSpace class need some better documentation. When I look up the signature for FunctionSpace, I get the following

fem.FunctionSpace(
    mesh: 'Mesh',
    element: 'typing.Union[ufl.FiniteElementBase, ElementMetaData]',
    cppV: 'typing.Optional[_cpp.fem.FunctionSpace]' = None,
    form_compiler_parameters: 'dict' = {},
    jit_parameters: 'dict' = {},
)
Docstring:      A space on which Functions (fields) can be defined.
Init docstring: Create a finite element function space.
File:           ~/local/src/fenicsx/dolfinx/python/dolfinx/fem/function.py
Type:           type
Subclasses:

In the demo the following line is written

V = FunctionSpace(mesh, ("Lagrange", 1))

and to me it seems like the second argument is a Tuple[str, int] and not typing.Union[ufl.FiniteElementBase, ElementMetaData]. I am not sure if you are running a static type checker (such as mypy or pyright) to make sure that the types are correct, but I guess that would have caught this. Also it would be good to have a little more description of the different arguments in the docstring. For example, I have no idea what ccpV would be. This is probably not needed for all functions, but the functions that are introduced in the beginner level demos should contain a bit more documentation in my opinion.

Use of lambda functions

Concerning the following line

facets = locate_entities_boundary(mesh, 1, lambda x: np.logical_or(np.isclose(x[0], 0.0),
                                                                   np.isclose(x[0], 2.0)))

While I think this is a good use case for a lambda function, I think it might add more complexity than it needs to (especially for beginners). For example rewriting this as

def marker(x):
    left = np.isclose(x[0], 0.0)
    right = np.isclose(x[0], 2.0)
    return np.logical_or(left, right)

facets = locate_entities_boundary(mesh, dim=1, marker=marker)

makes is a bit easier to understand in my opinion.

Plotting

I think it would also be good if the user could get some directions in how to install pyvista for plotting, i.e just print a message saying "To install pyvista do 'python -m pip install pyvista'" or something like that.

Hi,

I am a FEniCS user and I am currently exploring the FEniCSx demos in order to start learning the new syntax for FEniCSx. I started with the demo_possion.py, and I am sharing my thoughts here in order to express what I think could be be done in a different way, and what I think is currently missing. If this is not the correct format to provide such feedback please let me know. I would also like to say that many of the things I highlight here expresses my opinions which is definitely not necessarily the best for FEniCSx.

I would be happy to help out with a pull request if you think it is appropriate.

Thanks for the feedback. PRs would be welcome.

Imports

Do you have any guidelines in terms of import order? I think it is worth consider to have a consistent way of importing modules, such as first python std library, then third party and then dolfinx. Futhermore, within each group it should be alphabetical, and you can use a tool such as isort or reorder-python-imports to do this automatically (or as a pre-commit hook).

We're using isort, with settings at https://github.com/FEniCS/dolfinx/blob/main/python/.isort.cfg.

It should be some standard way in how you import modules and functions. For example, I think doing both
from dolfinx import fem
and
from dolfinx.fem import FunctionSpace
is a bit redundant. Why not just use fem.FunctionSpace?

Is there a recommended practice? Our preference has been to use fem.FunctionSpace, etc. Explanations for why things are currently organised a differently are (i) we refactored some modules and importing at the top saved having to make changes elsewhere (this has settled down now) and (ii) legacy.

One 'issue' is that we have a module called mesh, and mesh is also used commonly as a variable.

Similarly I think it would by good to just use e.g

   import ufl
   ufl.grad

Agree - an aim is to distinguish between UFL and DOLFINx.

I think this also aligns well with how people use other modules, such as numpy. This way there is no ambiguity when you use np.exp and ufl.exp for example.

Positional vs Keyword arguments

I don't know if you want to stick to using positional arguments. In my opinion it is more clear if one uses keyword arguments for most things. As an example, I think it is better to write
mesh = create_rectangle(
    comm=MPI.COMM_WORLD, 
    points=((0.0, 0.0), (2.0, 1.0)), 
    n=(32, 16), 
    cell_type=CellType.triangle
)

This would be fine with me.

than
mesh = create_rectangle(
    MPI.COMM_WORLD,
    ((0.0, 0.0), (2.0, 1.0)),
    (32, 16),
    CellType.triangle
)
FunctionSpace

I think it is great that you have started to use type hint in dolfinx. This will make it much better to work with in an IDE.

I think the FunctionSpace class need some better documentation. When I look up the signature for FunctionSpace, I get the following
fem.FunctionSpace(
    mesh: 'Mesh',
    element: 'typing.Union[ufl.FiniteElementBase, ElementMetaData]',
    cppV: 'typing.Optional[_cpp.fem.FunctionSpace]' = None,
    form_compiler_parameters: 'dict' = {},
    jit_parameters: 'dict' = {},
)
Docstring:      A space on which Functions (fields) can be defined.
Init docstring: Create a finite element function space.
File:           ~/local/src/fenicsx/dolfinx/python/dolfinx/fem/function.py
Type:           type
Subclasses:
In the demo the following line is written
V = FunctionSpace(mesh, ("Lagrange", 1))

Could you register a standalone issue for this?

and to me it seems like the second argument is a Tuple[str, int] and not typing.Union[ufl.FiniteElementBase, ElementMetaData]. I am not sure if you are running a static type checker (such as mypy or pyright) to make sure that the types are correct, but I guess that would have caught this.

Can you recommend one of the static checkers?

Also it would be good to have a little more description of the different arguments in the docstring. For example, I have no idea what ccpV would be. This is probably not needed for all functions, but the functions that are introduced in the beginner level demos should contain a bit more documentation in my opinion.

Use of lambda functions

Concerning the following line
facets = locate_entities_boundary(mesh, 1, lambda x: np.logical_or(np.isclose(x[0], 0.0),
                                                                   np.isclose(x[0], 2.0)))
While I think this is a good use case for a lambda function, I think it might add more complexity than it needs to (especially for beginners). For example rewriting this as
def marker(x):
    left = np.isclose(x[0], 0.0)
    right = np.isclose(x[0], 2.0)
    return np.logical_or(left, right)

facets = locate_entities_boundary(mesh, dim=1, marker=marker)
makes is a bit easier to understand in my opinion.

Plotting

I think it would also be good if the user could get some directions in how to install pyvista for plotting, i.e just print a message saying "To install pyvista do 'python -m pip install pyvista'" or something like that.

This would be helpful.

FEniCS / dolfinx