Skip to content

Fix inconsistent ufl usage in demos - #3532

Merged
garth-wells merged 25 commits into
FEniCS:mainfrom
schnellerhase:consistent_ufl_imports
Jan 11, 2025
Merged

Fix inconsistent ufl usage in demos#3532
garth-wells merged 25 commits into
FEniCS:mainfrom
schnellerhase:consistent_ufl_imports

Conversation

@schnellerhase

@schnellerhase schnellerhase commented Nov 24, 2024

Copy link
Copy Markdown
Contributor

Some examples use multiple import schemes of ufl, this switches to a single import style per demo.

For example currenltly demo_elasticity.py contains:

import ufl
from ufl import dx, grad, inner

Other examples that share this:

  • demo_biharmonic.py
  • demo_cahn-hilliard.py
  • demo_hdg.py
  • demo_helmholtz.py
  • demo_lagrange_variants.py
  • demo_poisson_matrix_free.py
  • demo_poisson.py
  • demo_pyamg.py
  • demo_stokes.py

@francesco-ballarin

Copy link
Copy Markdown
Member

Not sure if it is worth continuing to work on this.

Some of the developers are of the opinion that the choice of doing

from ufl import dx, grad, inner

is to ensure that the weak formulation of the problem is as close to the one on paper as possible, avoiding to have to add a prefix ufl. to every term in the formulation.
Personally that's not my opinion, but they do have a valid point.

@schnellerhase

Copy link
Copy Markdown
Contributor Author

Good point, but could we then switch to consistent usage of from ufl import ... in all cases?

@francesco-ballarin

Copy link
Copy Markdown
Member

I guess so, unless the list of imported names gets super long. If I were you I would put this on hold until a few other developers have had the opportunity to comment.

@jorgensd

Copy link
Copy Markdown
Member

I guess so, unless the list of imported names gets super long. If I were you I would put this on hold until a few other developers have had the opportunity to comment.

I think having a mixture doesn’t hurt.

@schnellerhase

Copy link
Copy Markdown
Contributor Author

I noticed this by chance when copying the elasticity weak form

2.0 * μ * ufl.sym(grad(v)) + λ * ufl.tr(ufl.sym(grad(v))) * ufl.Identity(len(v))

and it was not clear to me why it didn't just work with an ufl import that I spotted right away. I mean after taking a closer look at it was no problem, but it does seem odd to me to have different imports at play in one line for the same module.

@michalhabera

Copy link
Copy Markdown
Contributor

I noticed this by chance when copying the elasticity weak form

2.0 * μ * ufl.sym(grad(v)) + λ * ufl.tr(ufl.sym(grad(v))) * ufl.Identity(len(v))

and it was not clear to me why it didn't just work with an ufl import that I spotted right away. I mean after taking a closer look at it was no problem, but it does seem odd to me to have different imports at play in one line for the same module.

Here I agree it is very confusing. It dangerously suggests grad comes from some place other than sym. I think this should be made consistent per "module import level", i.e. the above example. Once importing from ufl import foo there should not be ufl.TrialFunction in the same file.

@schnellerhase

Copy link
Copy Markdown
Contributor Author

Updated all demos that shared this behavior to always follow the style used for the weak form, which I guess is the most important part of the ufl syntax.

@schnellerhase
schnellerhase marked this pull request as ready for review November 29, 2024 16:15

@michalhabera michalhabera left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. I would have the opinion to make it unified and do not use ufl.grad, ufl.sym, ..., but if others like to keep some demos with the explicit names then this is a good compromise.

Comment thread python/demo/demo_lagrange_variants.py
@garth-wells

Copy link
Copy Markdown
Member

Not sure if it is worth continuing to work on this.

Some of the developers are of the opinion that the choice of doing

from ufl import dx, grad, inner

is to ensure that the weak formulation of the problem is as close to the one on paper as possible, avoiding to have to add a prefix ufl. to every term in the formulation. Personally that's not my opinion, but they do have a valid point.

I quite like the explicitness of ufl.foo.

@garth-wells

Copy link
Copy Markdown
Member

Looks good. I would have the opinion to make it unified and do not use ufl.grad, ufl.sym, ..., but if others like to keep some demos with the explicit names then this is a good compromise.

If I had to pick, I prefer ufl.foo. It's then clear where the functions are coming from. Some names are close to or the same as NumPy functions, e.g. dot and inner, and we normally for np.foo.

@garth-wells garth-wells left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I find this confusing and would prefer ufl.foo. When I read, for example

c, mu = split(u)

it is extremely unclear if split is a dolfinx or a ufl function.

It would be better, in general, if imports from packages other than the 'home' package (dolfinx in the case of the demos) used namespacing.

@schnellerhase

Copy link
Copy Markdown
Contributor Author

When one decides for a consistent style one could also check for it https://docs.astral.sh/ruff/settings/#lint_flake8-import-conventions_banned-from

@michalhabera

Copy link
Copy Markdown
Contributor

Looks good. I would have the opinion to make it unified and do not use ufl.grad, ufl.sym, ..., but if others like to keep some demos with the explicit names then this is a good compromise.

If I had to pick, I prefer ufl.foo. It's then clear where the functions are coming from. Some names are close to or the same as NumPy functions, e.g. dot and inner, and we normally for np.foo.

I do not have a strong opinion here, so either way looks good. See also https://google.github.io/styleguide/pyguide.html#224-decision
More pressing is the inconsistency in a line, so I hope that fix gets merged.

@schnellerhase schnellerhase changed the title Fix inconsistent ufl usage in examples Fix inconsistent ufl usage in demos Dec 26, 2024
@schnellerhase

Copy link
Copy Markdown
Contributor Author

Switched to consistent and ruff checked usage of import ufl (disallows from ufl import ...) for python/demo/ - should we extend this to the whole of dolfinx? This would require a handful of more changes in the python unit tests.

@garth-wells garth-wells added demo New demo or demo enhancements/improvements housekeeping Tidying and style improvements python Pull requests that update Python code labels Jan 11, 2025
@garth-wells
garth-wells added this pull request to the merge queue Jan 11, 2025
Merged via the queue into FEniCS:main with commit 22a8ec2 Jan 11, 2025
@schnellerhase
schnellerhase deleted the consistent_ufl_imports branch January 11, 2025 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

demo New demo or demo enhancements/improvements housekeeping Tidying and style improvements python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants