Skip to content

Use class count, not instance - #52

Merged
connorjward merged 1 commit into
mainfrom
connorjward/fixup-count
Aug 27, 2026
Merged

connorjward merged 1 commit into
mainfrom
connorjward/fixup-count

Conversation

@connorjward

Copy link
Copy Markdown
Collaborator

No description provided.

@connorjward
connorjward merged commit 729712d into main Aug 27, 2026
2 checks passed
@pbrubeck

Copy link
Copy Markdown
Contributor

Follow-up: type(self).count += 1 reads the inherited value and writes the incremented one onto the subclass, so every subclass walks its own counter and prefixes still repeat across a hierarchy.

from petsctools import OptionsManager

class A(OptionsManager): pass
class B(A): pass

print([cls({}, default_prefix="firedrake_").options_prefix
       for cls in (A, B, A, B, OptionsManager, A)])
['firedrake_0_', 'firedrake_1_', 'firedrake_1_', 'firedrake_2_', 'firedrake_0_', 'firedrake_2_']

This is not hypothetical for Firedrake, which uses OptionsManager as a mixin:
NonlinearVariationalSolver(OptionsManager)LinearVariationalSolverLinearSolver,
and LinearEigensolver(OptionsManager). Building three of each in turn, against 729712d:

firedrake_1_ ['NVS']
firedrake_2_ ['LVS', 'NVS']                  <-- collision
firedrake_3_ ['LinearSolver', 'LVS', 'NVS']  <-- collision
firedrake_4_ ['LinearSolver', 'LVS']         <-- collision
firedrake_5_ ['LinearSolver']

So the option-inheritance hazard is weakened rather than removed: whether a solver built inside
another solver's inserted_options() picks up the outer solver's parameters now depends on how
many of each class were built beforehand. The tests that #52 turns green pass because that
particular pairing happens not to collide, not because collisions are gone — which makes the next
occurrence order-dependent and harder to pin down than the one this fixed.

Naming the class outright keeps one counter for the whole hierarchy:

options_prefix = f"{default_prefix}{OptionsManager.count}_"
OptionsManager.count += 1

Happy to reopen #53, which does that and adds a uniqueness assertion to test_options_prefix
(it fails on main either way), or leave it to you.

AI-assisted: found and checked with Claude Code.

@pbrubeck

Copy link
Copy Markdown
Contributor

I guess the collision will go away when NLVS stops inheriting from OptionsManager

@connorjward

Copy link
Copy Markdown
Collaborator Author

You make a great point. We should be using OptionsManager.count. Please do reopen

pbrubeck added a commit that referenced this pull request Aug 27, 2026
connorjward pushed a commit that referenced this pull request Aug 27, 2026
* Revert "Use class count, not instance (#52)"

This reverts commit 729712d.

* Give each autogenerated options prefix a unique index

`self.count += 1` reads the class attribute and rebinds the result onto
the instance, so `OptionsManager.count` stayed at zero and every manager
built without an explicit `options_prefix` was handed the same prefix,
`{default_prefix}0_`.

Sharing a prefix means sharing a namespace in the global options
database. A manager built while another manager's options are inserted
picks those options up as if they had been passed on the command line.
In Firedrake this makes any solver constructed inside a solve inherit
the outer solver's parameters: the mass solve behind
`Mesh.cell_sizes`, for instance, runs with the outer `-pc_type mg` and
then fails to coarsen a problem that has no hierarchy of its own.

Increment the counter on `OptionsManager` itself, so that one counter
serves the whole hierarchy. `type(self).count += 1` is not enough:
`OptionsManager` is used as a mixin -- Firedrake has
`NonlinearVariationalSolver` -> `LinearVariationalSolver` ->
`LinearSolver`, and `LinearEigensolver` -- and it reads the inherited
value but writes the incremented one onto the subclass, so each class
walks a counter of its own and prefixes repeat across the hierarchy:

    firedrake_2_ handed to both a LinearVariationalSolver and a
    NonlinearVariationalSolver

`test_options_prefix` builds two subclasses and the base class in turn
and checks the prefixes are distinct, which fails on either spelling of
the bug.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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