Skip to content

AssembledPC: fix sub_mat_type#5079

Open
pbrubeck wants to merge 4 commits intoreleasefrom
pbrubeck/fix/assembled-submat
Open

AssembledPC: fix sub_mat_type#5079
pbrubeck wants to merge 4 commits intoreleasefrom
pbrubeck/fix/assembled-submat

Conversation

@pbrubeck
Copy link
Copy Markdown
Contributor

@pbrubeck pbrubeck commented May 5, 2026

Description

@pbrubeck pbrubeck requested a review from connorjward May 5, 2026 09:26
@pbrubeck pbrubeck force-pushed the pbrubeck/fix/assembled-submat branch from e068707 to 2ffd089 Compare May 5, 2026 09:56
@pbrubeck pbrubeck force-pushed the pbrubeck/fix/assembled-submat branch from 2ffd089 to 2eb8a0e Compare May 5, 2026 09:57
Comment thread firedrake/solving_utils.py Outdated
pmat_type = pmat_type or self.pmat_type
for k, v in tuple(kwargs.items()):
if v is None:
kwargs.pop(k)
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.

This is really obscure. Why are those options disallowed?

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.

If we pass sub_mat_type=None we would like to grab the one from the parent _SNESContext. Basically this enables

sub_mat_type = kwargs.get("sub_mat_type") or self.sub_mat_type

but more generically

Copy link
Copy Markdown
Contributor Author

@pbrubeck pbrubeck May 5, 2026

Choose a reason for hiding this comment

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

The line below kwargs.setdefault("sub_mat_type", self.sub_mat_type) is a no-op if submat_type=None is already in the dictionary.

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.

Should the lines below be changed to something like

kwargs["sub_mat_type"] = kwargs.get("sub_mat_type") or self.sub_mat_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.

Yeah I think that's much better

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.

That will potentially cause conflicts with empty dictionaries or empty strings

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.

How so?

Copy link
Copy Markdown
Contributor Author

@pbrubeck pbrubeck May 5, 2026

Choose a reason for hiding this comment

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

We need

def set_default_kwarg(kwargs, key, default):
    if kwargs.get(key) is None:
        kwargs[key] = default

set_default_kwarg(kwargs, "sub_mat_type", self.sub_mat_type)
...

Copy link
Copy Markdown
Contributor Author

@pbrubeck pbrubeck May 5, 2026

Choose a reason for hiding this comment

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

How so?

reconstruct should keep the original arguments if they are not passed as kwargs or if they are set to None in the kwargs. Sometimes one might need to pass appctx={} or options_prefix="", which are falsy values that we should not treat as None.

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.

Explicitly filtering Nones like that is also fine. I just don't like the premature filtering of all kwarg options.

@pbrubeck pbrubeck force-pushed the pbrubeck/fix/assembled-submat branch from 3040d36 to 96ea75a Compare May 5, 2026 12:56
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