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

Added in a RestrictedFunctionSpace class #3426

Closed

Conversation

emmarothwell1
Copy link
Contributor

@emmarothwell1 emmarothwell1 commented Feb 26, 2024

Add in a RestrictedFunctionSpace class.

This marks boundary nodes in the definition of the FunctionSpace with the goal of removing them from calculations such as in assembly.
This currently only works for FunctionSpace objects, restricted mixed spaces and restrictions based on already defined RestrictedFunctionSpaces are still being worked on elsewhere.
There are tests in tests/regression/test_restricted_function_space.py to check assembly and solve works.
There is also a linked PyOP2 pull request at: https://github.com/OP2/PyOP2/pull/716

Copy link
Member

@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.

This is very close. Just a few minor issues.

firedrake/bcs.py Outdated
@@ -45,6 +46,8 @@ def __init__(self, V, sub_domain):
(isinstance(V.finat_element, finat.Hermite) and V.mesh().topological_dimension() > 1):
raise NotImplementedError("Strong BCs not implemented for element %r, use Nitsche-type methods until we figure this out" % V.finat_element)
self._function_space = V
if isinstance(sub_domain, (numbers.Integral, str)):
sub_domain = (sub_domain, )
Copy link
Member

Choose a reason for hiding this comment

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

Check if this is still needed.

# Get boundary points (if the boundary_set exists) and count each type
constrained_core = 0
constrained_owned = 0
boundary_points = np.array([])
Copy link
Member

Choose a reason for hiding this comment

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

Is this ever used.

@@ -2331,8 +2427,13 @@ def plex_renumbering(PETSc.DM plex,
CHKERR(DMLabelHasPoint(labels[l], p, &has_point))
if has_point:
PetscBTSet(seen, p)
perm[lidx[l]] = p
lidx[l] += 1
if boundary_set and PetscBTLookup(seen_boundary, p) and l <= 1:
Copy link
Member

Choose a reason for hiding this comment

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

please don't use the variable name l. It's super confusing.

return node_count*self.value_size

def local_to_global_map(self, bcs, lgmap=None):
return lgmap or self.dof_dset.lgmap
Copy link
Member

Choose a reason for hiding this comment

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

Let's discuss how much of this can go up to FunctionSpace.

@ksagiyam
Copy link
Contributor

ksagiyam commented Mar 1, 2024

This PR #3288 has just been merged. Rebasing will cause conflicts in assemble.py, so I suggest that you focus on fixing the boundary condition issue first. Once you resolve the BC issue, I can do rebasing.

@ksagiyam
Copy link
Contributor

ksagiyam commented Apr 4, 2024

It seems the big change in assemble.py has successfully been merged into this branch. The PR on halo_exchange_sf was merged yesterday, so you can drop the commit you cherry picked, and merge master.

@ksagiyam ksagiyam closed this Apr 22, 2024
@ksagiyam
Copy link
Contributor

Superseded #3215.

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.

3 participants