Skip to content

Conversation

@ksagiyam
Copy link

@ksagiyam ksagiyam commented Sep 28, 2025

Depends on:
ufl: FEniCS/ufl#429

Required for:
Firedrake: firedrakeproject/firedrake#4430

Use CellSequence in MixedElement.

@ksagiyam ksagiyam marked this pull request as draft September 28, 2025 13:43
@ksagiyam ksagiyam force-pushed the ksagiyam/introduce_mixed_map_mixed_cell branch from fb007d7 to 2bd2a36 Compare October 3, 2025 13:56
@ksagiyam ksagiyam force-pushed the ksagiyam/introduce_mixed_map_mixed_cell branch from 0b79221 to cf696fe Compare October 13, 2025 09:20
@ksagiyam ksagiyam force-pushed the ksagiyam/introduce_mixed_map_mixed_cell branch from cf696fe to e76d172 Compare October 15, 2025 19:56
cells = tuple(sorted(set(element.cell for element in elements) - set([None])))
self._cells = cells
cells = tuple(e.cell for e in elements)
if cells:

Choose a reason for hiding this comment

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

Suggested change
if cells:
if all(cell is not None for cell in cells):

Choose a reason for hiding this comment

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

Looks like we are always in the True branch, we could potentially delete the False branch

Copy link
Author

Choose a reason for hiding this comment

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

Turned out elements can be () when doing something with VertexOnlyMesh.

__slots__ = ("_sub_elements", "_cells")

def __init__(self, *elements, **kwargs):
def __init__(self, *elements, make_cell_sequence=True, **kwargs):

Choose a reason for hiding this comment

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

Any good reason to introduce this kwarg? Could we not always create a CellSequence when the elements are on different cells?

Copy link
Author

Choose a reason for hiding this comment

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

We make CellSequence for proper MixedElements, but not for VectorElement or TensorElement.

Copy link

@connorjward connorjward Oct 29, 2025

Choose a reason for hiding this comment

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

What if we had:

elem = MixedElement(...)
elem.cells  # always gives a CellSequence
elem.cell  # crashes if elem.cells are not all the same cell, otherwise returns the unique cell

Copy link
Author

Choose a reason for hiding this comment

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

Please see diff.

Choose a reason for hiding this comment

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

Definitely better.

@ksagiyam ksagiyam force-pushed the ksagiyam/introduce_mixed_map_mixed_cell branch 3 times, most recently from b1b64df to fdc993a Compare October 29, 2025 23:29
@ksagiyam ksagiyam force-pushed the ksagiyam/introduce_mixed_map_mixed_cell branch from fdc993a to 752703e Compare October 31, 2025 14:08
@ksagiyam ksagiyam marked this pull request as ready for review October 31, 2025 14:14
@ksagiyam
Copy link
Author

Merging as all discussions have been resolved.

@ksagiyam ksagiyam merged commit 9968afb into main Oct 31, 2025
8 checks passed
@ksagiyam ksagiyam deleted the ksagiyam/introduce_mixed_map_mixed_cell branch October 31, 2025 14:17
@connorjward
Copy link

Merging as all discussions have been resolved.

Please try to get an approving review before merging.

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