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

Expunge Expr.ufl_domain Part II #3259

Merged
merged 12 commits into from Nov 29, 2023
Merged

Expunge Expr.ufl_domain Part II #3259

merged 12 commits into from Nov 29, 2023

Conversation

pbrubeck
Copy link
Contributor

@pbrubeck pbrubeck commented Nov 23, 2023

Description

This was still triggering warnings at interpolation (and when trying to output non-Lagrange functions).

connorjward
connorjward previously approved these changes Nov 23, 2023
Copy link
Contributor

@connorjward connorjward left a comment

Choose a reason for hiding this comment

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

Looks sensible.

@JDBetteridge
Copy link
Member

Hurrah, now we are only raising 13414 warnings on CI!

This is a massive improvement over the 20947 warnings we generated previously.

@pbrubeck do you fancy opening a similar PR to expunge this warning from UFL itself?

@pbrubeck
Copy link
Contributor Author

pbrubeck commented Nov 23, 2023

Hurrah, now we are only raising 13414 warnings on CI!

This is a massive improvement over the 20947 warnings we generated previously.

@pbrubeck do you fancy opening a similar PR to expunge this warning from UFL itself?

I am not entirely sure we are completely eliminating these warnigns. Maybe I'll first open a ufl PR that fails instead of raising the warning to detect where else we need to change our code, and then open another PR to remove the warning.

@pbrubeck pbrubeck changed the title Expunge Coefficient.ufl_domain Expunge Expr.ufl_domain Part II Nov 23, 2023
@wence-
Copy link
Contributor

wence- commented Nov 23, 2023

Hurrah, now we are only raising 13414 warnings on CI!
This is a massive improvement over the 20947 warnings we generated previously.
@pbrubeck do you fancy opening a similar PR to expunge this warning from UFL itself?

I am not entirely sure we are completely eliminating these warnigns. Maybe I'll first open a ufl PR that fails instead of raising the warning to detect where else we need to change our code, and then open another PR to remove the warning.

you can either turn warnings into errors in pytest, or, if you run just a python script, do python -Werror --pdb script.py to turn warnings into errors and drop into the debugger: https://docs.python.org/3/using/cmdline.html#cmdoption-W

.github/workflows/build.yml Outdated Show resolved Hide resolved
@pbrubeck
Copy link
Contributor Author

pbrubeck commented Nov 23, 2023

We went down to 1249 warnings after making sure this warning is never triggered by the tests.

connorjward
connorjward previously approved these changes Nov 24, 2023
Copy link
Contributor

@connorjward connorjward left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to do this.

Copy link
Contributor

@connorjward connorjward left a comment

Choose a reason for hiding this comment

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

Just clearing up a strange bit of code. Otherwise fine.

firedrake/preconditioners/patch.py Outdated Show resolved Hide resolved
Co-authored-by: Connor Ward <c.ward20@imperial.ac.uk>
dham
dham previously approved these changes Nov 29, 2023
@ReubenHill ReubenHill enabled auto-merge (squash) November 29, 2023 16:32
@ReubenHill ReubenHill merged commit 62f6bf7 into master Nov 29, 2023
10 checks passed
@ReubenHill ReubenHill deleted the pbrubeck/fix/ufl_domain branch November 29, 2023 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants