Skip to content

fix direct assembly with equation_bcs#1726

Merged
wence- merged 2 commits intomasterfrom
fix_equation_bc
Jun 26, 2020
Merged

fix direct assembly with equation_bcs#1726
wence- merged 2 commits intomasterfrom
fix_equation_bc

Conversation

@ksagiyam
Copy link
Copy Markdown
Contributor

@ksagiyam ksagiyam commented Jun 5, 2020

This PR fixes some equation_bc related bugs: see #1725.

Comment thread firedrake/bcs.py Outdated
elif form_type == 'Jp':
return tuple(bc if isinstance(bc, DirichletBC) else bc._Jp for bc in bcs)
else:
raise TypeError("Unknown form_type: 'form_type' must be 'F', 'J', or 'Jp'.")
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.

Given you're using strings, just do:

if form_type not in {"F", "J", "Jp"}:
    raise ValueError(...)
else:
   return tuple(bc if isinstance(bc, DirichletBC) else getattr(bc, f"_{form_type}") for bc in bcs)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks.


assemble(a, bcs=extract_equation_bc_forms(bc, 'J'))
assemble(a, bcs=extract_equation_bc_forms([bc], 'Jp'))
with pytest.raises(RuntimeError) as excinfo:
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.

Change the raise to ValueError (it's not a TypeError) and then assert that you get a ValueError here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed TypeError -> ValueError in bc.py.
Here assemble(..., bcs=bc) should actually complain that a raw (unpreprocessed) EquationBC object is given instead of a EquationBCSplit object.
Tried to remove ambiguity in the error message and added more comments.

bc = EquationBC(F1 == 0, u, 1)

assemble(F, bcs=extract_equation_bc_forms(bc, 'F'))
assemble(F, bcs=bc)
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.

Can we check for something more than "the code runs"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Now checks assembled values.

bc = EquationBC(a1 == L1, u, 1)

assemble(a, bcs=extract_equation_bc_forms(bc, 'J'))
assemble(a, bcs=extract_equation_bc_forms([bc], 'Jp'))
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.

Likewise here, can we check that we got the right values?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here, too.

Comment thread firedrake/assemble.py Outdated
Preprocess `bcs` either with: \
`bcs = extract_equation_bc_forms(bcs, 'J')`, \
or with \
`bcs = extract_equation_bc_forms(bcs, 'Jp')`.")
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.

Prefer to paste strings like this:

raise RuntimeError("Part of a long string: "
                                 "Another part ")

Also, this message will be very long.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread firedrake/bcs.py Outdated
raise TypeError("homogenize only takes a DirichletBC or a list/tuple of DirichletBCs")


def extract_equation_bc_forms(bcs, form_type):
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.

Make this a method on EquationBC? then just call it extract forms?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@wence- wence- force-pushed the fix_equation_bc branch from bf3f26b to bca8c07 Compare June 23, 2020 16:05
@wence- wence- force-pushed the fix_equation_bc branch from bca8c07 to 4108633 Compare June 23, 2020 16:06
@wence-
Copy link
Copy Markdown
Contributor

wence- commented Jun 23, 2020

I have rebased on top of master (fixing the merge conflicts) please have a check.

@ksagiyam
Copy link
Copy Markdown
Contributor Author

Thanks!!
Attempted to clarify things slightly.

@wence- wence- merged commit 2a8aea3 into master Jun 26, 2020
@wence- wence- deleted the fix_equation_bc branch June 26, 2020 10:21
@wence-
Copy link
Copy Markdown
Contributor

wence- commented Jun 26, 2020

thanks.

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.

2 participants