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.

Sign up for GitHub

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

Jump to bottom

Add __str__ method for SolverVarFormBlock #3199

Closed
wants to merge 2 commits into from
Closed

Add __str__ method for SolverVarFormBlock #3199

wants to merge 2 commits into from

Conversation

Ig-dolci
Copy link
Contributor

@Ig-dolci Ig-dolci commented Nov 2, 2023 •
edited
Loading

Description

Adding __str__ method in the SolverVarFormBlock.

The __str__ is useful to visualise the tape DAG.

The ufl2unicode(self.lhs) is going to fail raised by a ufl bug. I have opened a UFL PR to fix it.

Obs:
Thank you Connor for helping me with the UFL fail.:)

@Ig-dolci Ig-dolci marked this pull request as ready for review November 2, 2023 16:57
JDBetteridge
JDBetteridge previously requested changes Nov 2, 2023
View reviewed changes
firedrake/adjoint_utils/blocks/solving.py Outdated Show resolved Hide resolved
@connorjward
Copy link
Contributor

I will mark this PR as draft for now since we want to land the UFL PR first.

@connorjward connorjward marked this pull request as draft November 3, 2023 09:33
@connorjward connorjward marked this pull request as ready for review November 3, 2023 15:31
@connorjward
Copy link
Contributor

We need to update our UFL fork now. This might be complicated since there is already a longstanding PR to do this. @nbouziani what is the status of that?

@nbouziani
Copy link
Contributor

nbouziani commented Nov 3, 2023 •
edited
Loading

We need to update our UFL fork now. This might be complicated since there is already a longstanding PR to do this. @nbouziani what is the status of that?

That PR was for the dual changes and is out-of-date given that we merged UFL upstream after that. However, I recently noticed that the recent UFL changes with the new finite element interface have caused some of our tests to fail. There seem to be minor things to change first in firedrake and tsfc to update our UFL fork successfully.

@connorjward
Copy link
Contributor

We need to update our UFL fork now. This might be complicated since there is already a longstanding PR to do this. @nbouziani what is the status of that?

That PR was for the dual changes and is out-of-date given that we merged UFL upstream after that. However, I recently noticed that the recent UFL changes with the new finite element interface have caused some of our tests to fail. There seem to be minor things to change first in firedrake and tsfc to update our UFL fork successfully.

Is someone willing to take that on? @nbouziani @Ig-dolci?

@Ig-dolci
Copy link
Contributor Author

Ig-dolci commented Nov 7, 2023

We need to update our UFL fork now. This might be complicated since there is already a longstanding PR to do this. @nbouziani what is the status of that?

That PR was for the dual changes and is out-of-date given that we merged UFL upstream after that. However, I recently noticed that the recent UFL changes with the new finite element interface have caused some of our tests to fail. There seem to be minor things to change first in firedrake and tsfc to update our UFL fork successfully.

Is someone willing to take that on? @nbouziani @Ig-dolci?

I will work on this today.

@Ig-dolci
Copy link
Contributor Author

Ig-dolci commented Nov 7, 2023

We need to update our UFL fork now. This might be complicated since there is already a longstanding PR to do this. @nbouziani what is the status of that?

That PR was for the dual changes and is out-of-date given that we merged UFL upstream after that. However, I recently noticed that the recent UFL changes with the new finite element interface have caused some of our tests to fail. There seem to be minor things to change first in firedrake and tsfc to update our UFL fork successfully.

Is someone willing to take that on? @nbouziani @Ig-dolci?

I will work on this today.

I just figured out that these PRs 3166 and 302 are aiming to update firedrake and tsfc for the latest ufl (upstream) interface.

dham
dham reviewed Nov 8, 2023
View reviewed changes
firedrake/adjoint_utils/blocks/solving.py
@@ -600,6 +600,9 @@ def _init_solver_parameters(self, args, kwargs):
super()._init_solver_parameters(args, kwargs)
solve_init_params(self, args, kwargs, varform=True)

def __str__(self):
return f"solve({ufl2unicode(self.lhs)} == {ufl2unicode(self.rhs)})"
Copy link
Member

Choose a reason for hiding this comment

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

Need to cast the rhs to ufl.

@Ig-dolci
Copy link
Contributor Author

This PR is incorrect. I did not realise that super has the __str__ method. I will close this PR, but I opened the PR 3222 aiming to fix the error for the cases in which rhs is a Cofunction type.

@Ig-dolci Ig-dolci closed this Nov 10, 2023
@Ig-dolci Ig-dolci deleted the dolci/SolverVarFormBlock_str branch November 27, 2023 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JDBetteridge JDBetteridge JDBetteridge left review comments

@dham dham dham left review comments

@connorjward connorjward connorjward approved these changes

Assignees

@connorjward connorjward

@JDBetteridge JDBetteridge

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ig-dolci @connorjward @nbouziani @dham @JDBetteridge