Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix action space check #331

Merged

Conversation

pbrubeck
Copy link
Contributor

@pbrubeck pbrubeck commented Dec 12, 2024

The space check in action was not quite right, it is hard to tell exactly why. Previously it only passed because V == V.dual() in Firedrake.

This PR removes the need to check specific types of the arguments and handles them in a more symmetric way.

@pbrubeck pbrubeck force-pushed the pbrubeck/fix/action-space-check branch from 4e6729b to 59820fc Compare December 12, 2024 10:53
@pbrubeck pbrubeck force-pushed the pbrubeck/fix/action-space-check branch from 59820fc to f2a9c8a Compare December 12, 2024 10:54
@pbrubeck
Copy link
Contributor Author

This ruff thing is ridiculous

@jorgensd
Copy link
Member

This ruff thing is ridiculous

You can add:
# noqa: RUF023 in the few places where you don’t want ruff sorting (where the order is specified by the comments).

ruff enforces code style simeq to black, which makes life easier for contributors as they can run ruff format and ruff check to ensure that everyone uses a consistent code style. This also reduces the diff’s for each PR (except when ruff adds new rules).

see for instance #330
where I’ve applied ruff and one noqa where I don’t want to sort slots.

@@ -159,29 +157,36 @@ def __hash__(self):

def _check_function_spaces(left, right):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dear code reviewers,

Please ignore the rest of the diff, that's just ruff.
Only take a look at this function.

Thank you.

@pbrubeck
Copy link
Contributor Author

@jorgensd FeniCSx CI fails, but I suspect it is for an unrelated reason?

@jorgensd
Copy link
Member

@jorgensd FeniCSx CI fails, but I suspect it is for an unrelated reason?

@finsberg, it seems like you missed a test with your latest PR:
https://github.com/FEniCS/dolfinx/blob/0c3a33ec5cfc5fef3e5bc965fcd3df533ed4aee6/python/test/unit/io/test_xdmf_function.py#L232-L249

Copy link
Collaborator

@dham dham left a comment

Choose a reason for hiding this comment

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

I think this looks right.

@@ -9,15 +9,14 @@

from itertools import chain

from ufl import matrix # noqa 401
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ufl.matrix wants to be imported before ufl.action. Also note that ufl.action.Action is the first class to be imported within __init__.py

@mscroggs mscroggs added this pull request to the merge queue Dec 13, 2024
Merged via the queue into FEniCS:main with commit 9e8a5bf Dec 13, 2024
12 checks passed
@mscroggs mscroggs deleted the pbrubeck/fix/action-space-check branch December 13, 2024 14:26
pbrubeck pushed a commit to firedrakeproject/ufl that referenced this pull request Dec 18, 2024
Fix action space check

ruff

unsafe ruff

symmetric space check

fix import conflict

Fix action space check (FEniCS#331)

* Fix action space check

* ruff

* unsafe ruff

* symmetric space check

* fix import conflict

remove apply_default_restrictions() (FEniCS#329)

* ruff==0.8.0

* remove apply_default_restrictions()

---------

Co-authored-by: Matthew Scroggs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants