Ehancement: Named AgentSet + flexible lookup & stricter separation of concerns
#172
Conversation
…ed agent set management
…oved performance and clarity
…roved clarity and performance
…od for AgentSetsAccessor
…ement and enhance rename method for better delegation to AgentsDF.
…messages for key lookups
…ames in ValueError for better debugging
…gent sets as a list for multiple matches and improve error messaging for better clarity.
…ct functionality and error handling
…o 146-enhancement-consider-using-a-key-based-structure-for-agentsets-instead-of-list-in-agentsdf
…ency and clarity in the abstract class naming.
…proved consistency and clarity; enhance error messaging in __getitem__ and rename methods for better readability.
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #172 +/- ##
==========================================
- Coverage 92.54% 88.85% -3.69%
==========================================
Files 14 14
Lines 1717 1983 +266
==========================================
+ Hits 1589 1762 +173
- Misses 128 221 +93 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nt set types, improving performance and clarity in KeyError messages.
…arity in AGENTS.md, agents.py, agentset.py, and test_sets_accessor.py; improve formatting in test cases.
…r-agentsets-instead-of-list-in-agentsdf' of https://github.com/projectmesa/mesa-frames into 146-enhancement-consider-using-a-key-based-structure-for-agentsets-instead-of-list-in-agentsdf
…cessor; update parameter descriptions for improved understanding.
…update conflict resolution and mode descriptions for improved understanding.
…type annotations for parameters.
…cessor and AgentSetsAccessor; update default values and descriptions for parameters.
for more information, see https://pre-commit.ci
… conflict handling
…r improved clarity
…nality across multiple classes
…r-agentsets-instead-of-list-in-agentsdf' of https://github.com/projectmesa/mesa-frames into 146-enhancement-consider-using-a-key-based-structure-for-agentsets-instead-of-list-in-agentsdf
… DataCollector examples
…o 146-enhancement-consider-using-a-key-based-structure-for-agentsets-instead-of-list-in-agentsdf
for more information, see https://pre-commit.ci
…files These files contained outdated implementations of the AgentSetsAccessor and AgentsDF classes, which are no longer in use. Their removal helps to clean up the codebase and reduce confusion regarding the current architecture of the mesa-frames library.
…nforce model consistency
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughRefactors agent-set APIs toward a registry-centric model: abstract and concrete AgentSet/Registry interfaces redesigned; Space accepts AbstractAgentSet inputs; DataCollector reporter semantics updated; Model step delegates via registry and adds step counter; docs and examples updated; tests restructured to new APIs, including extensive new registry tests and removal of legacy suite. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Model
participant Registry as AgentSetRegistry
participant SetA as AgentSet A
participant SetB as AgentSet B
User->>Model: step()
note over Model: _wrapped_step increments m.steps
Model->>Registry: do("step")
Registry->>SetA: step()
SetA-->>Registry: None
Registry->>SetB: step()
SetB-->>Registry: None
Registry-->>Model: None
Model-->>User: return
sequenceDiagram
autonumber
participant Caller
participant Space
participant Reg as AbstractAgentSetRegistry
participant ASet as AbstractAgentSet
Caller->>Space: move_agents(agents=ASet, pos=...)
Space->>Space: _get_ids_srs(ASet)
note over Space: Validate ids ∈ Reg.ids
Space->>Space: _place_or_move_agents_to_cells(...)
Space-->>Caller: Self
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
mesa_frames/concrete/datacollector.py (1)
458-462: Fix “postgress” typos and includebatchas a required column.Typos reduce discoverability; missing
batchvalidation can lead to runtime INSERT errors.Apply this diff:
- self._validate_postgress_table_exists(conn) - self._validate_postgress_columns_exists(conn) + self._validate_postgres_table_exists(conn) + self._validate_postgres_columns_exists(conn) @@ - def _validate_postgress_table_exists(self, conn: connection): + def _validate_postgres_table_exists(self, conn: connection): @@ - def _validate_postgress_columns_exists(self, conn: connection): + def _validate_postgres_columns_exists(self, conn: connection):And update required columns:
- required_columns = { - "step": "Integer", - "seed": "Varchar", - } + required_columns = { + "step": "Integer", + "seed": "Varchar", + "batch": "Integer", + }Also applies to: 465-478, 479-496
mesa_frames/types_.py (1)
45-53: Runtime NameError when pandas is missing (pd.DataFrame referenced).PolarsDataFrameInput refers to pd.DataFrame directly. If pandas is unavailable, pd is undefined.
Use the alias defined above to avoid runtime failures:
PolarsDataFrameInput = Union[ Mapping[ str, Union[Sequence[object], Mapping[str, Sequence[object]], pl.Series, Any] ], Sequence[Any], np.ndarray, ArrowTable, - pd.DataFrame, + PandasDataFrame, ]mesa_frames/abstract/space.py (9)
232-258: swap_agents can mis-pair agents due to ordering; reindex masked DFs before reassignment.
_df_get_masked_dfmay not preserve the mask order. Without explicit reindexing, agent IDs can be swapped against the wrong counterparts.Apply:
agents0_df = obj._df_get_masked_df( obj._agents, index_cols="agent_id", mask=ids0 ) + agents0_df = obj._df_reindex(agents0_df, ids0, "agent_id") agents1_df = obj._df_get_masked_df( obj._agents, index_cols="agent_id", mask=ids1 ) + agents1_df = obj._df_reindex(agents1_df, ids1, "agent_id") agents0_df = obj._df_set_index(agents0_df, "agent_id", ids1) agents1_df = obj._df_set_index(agents1_df, "agent_id", ids0)
491-527: Indexing a generic Collection breaks for sets/generators; normalize to list and guard mixed types.
agents[0]onCollectionraises for non-sequences; also allows mixed container types silently.- elif isinstance(agents, Collection) and ( - isinstance(agents[0], AbstractAgentSet) - or isinstance(agents[0], AbstractAgentSetRegistry) - ): - ids = [] - for a in agents: + elif isinstance(agents, Collection) and not isinstance(agents, (str, bytes)): + seq = list(agents) + if not seq: + return self._srs_constructor([], name="agent_id", dtype="uint64") + ids: list[Series] = [] + for a in seq: if isinstance(a, AbstractAgentSet): ids.append( self._srs_constructor( self._df_index(a.df, "unique_id"), name="agent_id", dtype="uint64", ) ) - elif isinstance(a, AbstractAgentSetRegistry): + elif isinstance(a, AbstractAgentSetRegistry): ids.append( self._srs_constructor(a.ids, name="agent_id", dtype="uint64") ) + else: + raise TypeError("Mixed collection of agent containers is not supported") return self._df_concat(ids, ignore_index=True)
1345-1349: Broaden get_neighbors(agents=...) to accept AbstractAgentSet, matching base API.Current signature narrows accepted types vs Space.get_neighbors and _get_df_coords.
- agents: IdsLike - | AbstractAgentSetRegistry - | Collection[AbstractAgentSetRegistry] + agents: IdsLike + | AbstractAgentSet + | AbstractAgentSetRegistry + | Collection[AbstractAgentSet] + | Collection[AbstractAgentSetRegistry] | None = None,
1362-1369: Broaden get_neighborhood(agents=...) to accept AbstractAgentSet.Keep subclass surface consistent with base and helpers.
- agents: IdsLike - | AbstractAgentSetRegistry - | Collection[AbstractAgentSetRegistry] - | None = None, + agents: IdsLike + | AbstractAgentSet + | AbstractAgentSetRegistry + | Collection[AbstractAgentSet] + | Collection[AbstractAgentSetRegistry] + | None = None,
1372-1392: Avoid isinstance(..., ArrayLike): use Sequence checks to prevent runtime TypeError.
ArrayLikeis a typing alias and not safe for isinstance. Use Sequence and exclude str/bytes.- if __debug__: - if isinstance(radius, ArrayLike): + if __debug__: + if isinstance(radius, Sequence) and not isinstance(radius, (str, bytes)): if len(radius) != len(pos_df): raise ValueError( "The length of the radius sequence must be equal to the number of positions/agents" ) @@ - if isinstance(radius, ArrayLike): + if isinstance(radius, Sequence) and not isinstance(radius, (str, bytes)): radius_srs = self._srs_constructor(radius, name="radius") radius_df = self._srs_to_df(radius_srs) max_radius = radius_srs.max() else: max_radius = radius @@ - if isinstance(radius, ArrayLike): + if isinstance(radius, Sequence) and not isinstance(radius, (str, bytes)): radius_df = self._df_rename_columns( self._df_concat([pos_df, radius_df], how="horizontal"), self._pos_col_names + ["radius"], self._center_col_names + ["max_radius"], )Also, this line likely needs
stop=(see next comment):- range_df = self._srs_to_df( - self._srs_range(name="radius", start=1, end=max_radius + 1) - ) + range_df = self._srs_to_df( + self._srs_range(name="radius", start=1, stop=max_radius + 1) + )Also applies to: 1486-1502
1831-1843: Don’t mutate tuple/sequence in-place; convert to list first. Also prefer Sequence over ArrayLike in isinstance.In-place assignment on tuples raises; using Sequence keeps runtime-safe checks.
- elif isinstance(pos, ArrayLike) and len(pos) == len(self._dimensions): + elif isinstance(pos, Sequence) and not isinstance(pos, (str, bytes)) and len(pos) == len(self._dimensions): # This means that the sequence is already a sequence where each element is the # sequence of coordinates for dimension i - for i, c in enumerate(pos): + pos_list = list(pos) + for i, c in enumerate(pos_list): if isinstance(c, slice): start = c.start if c.start is not None else 0 step = c.step if c.step is not None else 1 stop = c.stop if c.stop is not None else self._dimensions[i] - pos[i] = self._srs_range(start=start, stop=stop, step=step) + pos_list[i] = self._srs_range(start=start, stop=stop, step=step) return self._df_constructor( - data=[pos], + data=[pos_list], columns=self._pos_col_names, dtypes={col: int for col in self._pos_col_names}, )
1372-1392: ArrayLike is a PEP 604 union — replace isinstance(..., ArrayLike) checkstypes_.py (line 85) defines ArrayLike = ndarray | Series | Sequence — this is a runtime UnionType and cannot be passed to isinstance; those checks will raise TypeError.
Affected call sites: mesa_frames/abstract/space.py:1373, 1382, 1486, 1830–1831.
Fix: replace isinstance(x, ArrayLike) with isinstance(x, (ndarray, Series, Sequence)) or add an explicit is_arraylike(obj) helper that checks the concrete types (and import them).
1298-1339: LGTM — distance/direction normalization good; fix inconsistent signatures.Abstract declarations (mesa_frames/abstract/space.py:350, 858) accept radius: int | float | Sequence[...] | ArrayLike, while later definitions (mesa_frames/abstract/space.py:1341, 1360) narrow radius to int | Sequence[int]; align the signatures or document/justify the narrowing to avoid API/typing breakage.
1390-1392: Fix _srs_range keyword mismatch (end vs stop)abstract/mixin.py defines _srs_range(..., end: int, ...); space.py has calls using both end= (line ~1390) and stop= (line ~1838). Update the call at mesa_frames/abstract/space.py:1838 to use end= (or change the method signature to accept stop=) so all call sites and the implementation use the same keyword to avoid a TypeError at runtime.
🧹 Nitpick comments (37)
tests/conftest.py (1)
5-7: Determinism: set the env var unconditionally (docstring vs behavior).Docstring says we ensure type checking is enabled; setdefault won’t override an existing “0”. Set it explicitly to avoid nondeterminism across dev machines/CI.
-os.environ.setdefault("MESA_FRAMES_RUNTIME_TYPECHECKING", "1") +os.environ["MESA_FRAMES_RUNTIME_TYPECHECKING"] = "1"Also applies to: 11-11
tests/test_grid.py (1)
15-17: Handle empty registries in get_unique_ids.pl.concat([]) raises; make helper robust for models with no sets.
- series_list = [aset["unique_id"].cast(pl.UInt64) for aset in model.sets] - return pl.concat(series_list) + series_list = [aset["unique_id"].cast(pl.UInt64) for aset in model.sets] + return pl.concat(series_list) if series_list else pl.Series("unique_id", [], dtype=pl.UInt64)examples/sugarscape_ig/ss_polars/agents.py (1)
38-46: Scope fix LGTM; add dtype guard and micro‑simplify.Cast ids to UInt64 to match unique_id dtype and combine filters to avoid a temporary mask.
- # Only consider cells currently occupied by agents of this set - cells = self.space.cells.filter(pl.col("agent_id").is_not_null()) - mask_in_set = cells["agent_id"].is_in(self.index) - if mask_in_set.any(): - cells = cells.filter(mask_in_set) - ids = cells["agent_id"] + # Only consider cells occupied by agents of this set + cells = self.space.cells.filter( + pl.col("agent_id").is_not_null() & pl.col("agent_id").is_in(self.index) + ) + if len(cells) > 0: + ids = cells["agent_id"].cast(pl.UInt64) self[ids, "sugar"] = ( self[ids, "sugar"] + cells["sugar"] - self[ids, "metabolism"] )examples/sugarscape_ig/ss_polars/model.py (1)
36-40: Main-set lookup can break after rename; keep a direct reference.Storing only the name is brittle. Persist the object for termination checks.
main_set = agent_type(self, n_agents, initial_sugar, metabolism, vision) self.sets += main_set self._main_set_name = main_set.name + self._main_set = main_set # robust to renamesdocs/general/user-guide/2_introductory-tutorial.ipynb (1)
77-79: Docs: named-set key may vary with canonicalization.If the set name collides, it becomes MoneyAgents_1. Consider noting this or deriving the name programmatically in examples to avoid KeyError in reused kernels.
- "total_wealth": lambda m: m.sets["MoneyAgents"].df["wealth"].sum() + "total_wealth": lambda m: m.sets["MoneyAgents"].df["wealth"].sum() # if renamed, use next(iter(m.sets)).dfmesa_frames/abstract/datacollector.py (1)
94-99: Doc update aligns with new reporter semantics.Clear and helpful. Consider qualifying types in text (e.g., pl.Series/pl.DataFrame) for readers.
docs/general/user-guide/4_datacollector.ipynb (1)
123-125: Docs: consistent MoneyAgents lookups; add note for renamed sets.Using m.sets["MoneyAgents"] is fine; add a brief note that in multi-set demos names may canonicalize (MoneyAgents_1), or show an alternative using the first set to keep examples copy-paste proof.
Also applies to: 201-203, 252-254, 293-295
docs/general/user-guide/1_classes.md (2)
30-33: Clarify naming/defaults when adding sets.Suggest adding a sentence that
+=/add()registers the set under its default name (class name) unless explicitly overridden, and show retrieval by that name for consistency with later examples.
80-92: LGTM: DataCollector example matches new registry/set-centric reporters.Minor suggestion: mirror the trigger example from tests/docs to use
m.steps(avoid private_steps).mesa_frames/concrete/model.py (1)
97-101: Duplicatestepsproperty; keep a single definition.Two identical
@property stepsblocks exist. Retain one to prevent confusion and doc tool noise.Apply this diff to remove the earlier duplicate:
@@ - @property - def steps(self) -> int: - """Get the current step count.""" - return self._stepsAlso applies to: 132-142
tests/test_datacollector.py (2)
11-13: Prefer publicstepsover private_stepsin trigger.Use
model.steps % 2 == 0to avoid reaching into internals.Apply this diff:
-def custom_trigger(model): - return model._steps % 2 == 0 +def custom_trigger(model): + return model.steps % 2 == 0
733-734: Fix assertion message to match expected count.Message says 4 files but the assertion expects 8.
Apply this diff:
- assert len(created_files) == 8, ( - f"Expected 4 files, found {len(created_files)}: {created_files}" + assert len(created_files) == 8, ( + f"Expected 8 files, found {len(created_files)}: {created_files}" )mesa_frames/concrete/datacollector.py (3)
181-213: Deduplicate_is_str_collectionand make it exception-free.Define it once (e.g., as a
@staticmethod) and avoid broad try/except.Apply this minimal change in-place:
- def _is_str_collection(x: Any) -> bool: - try: - from collections.abc import Collection - - if isinstance(x, str): - return False - return isinstance(x, Collection) and all(isinstance(i, str) for i in x) - except Exception: - return False + from collections.abc import Collection + def _is_str_collection(x: Any) -> bool: + return ( + not isinstance(x, str) + and isinstance(x, Collection) + and all(isinstance(i, str) for i in x) + )
452-456: TightenNonecheck and fix message grammar.Apply this diff:
- if self._storage != "memory" and self._storage_uri == None: + if self._storage != "memory" and self._storage_uri is None: raise ValueError( - "Please define a storage_uri to if to be stored not in memory" + "Please define storage_uri to store data outside memory" )
497-523: Prefer parametrized identifiers or safe quoting for identifiers.While
schema/tablecome from controlled values, consider psycopg2sql.Identifierto guard against edge cases in names.tests/test_agentsetregistry.py (3)
271-275: Replace tautological assertion with a meaningful check.
assert reg[1].name == reg[1].nameis a no-op. Verify mapping coherence or the replacement instead.Apply this diff:
- assert reg[1].name == reg[1].name + assert reg.get(reg[1].name) is reg[1]
313-313: Adopt Ruff hint for first value retrieval.Use an iterator instead of list materialization.
Apply this diff:
- assert list(reg.values())[0] is reg[0] + assert next(iter(reg.values())) is reg[0]
362-374: Strengthen deep-copy assertion in__add__test.Optionally assert identity inequality to ensure copies, e.g.,
reg_new[0] is not a1.mesa_frames/types_.py (3)
19-26: Define PandasSeries when pandas is available.When pandas is installed, PandasSeries isn’t defined, leading to asymmetric aliases vs. the ImportError path.
Apply this diff:
try: import pandas as pd PandasDataFrame = pd.DataFrame + PandasSeries = pd.Series except ImportError: # just give us a class so annotations don’t break PandasDataFrame = type("PandasDataFrame", (), {}) PandasSeries = type("PandasSeries", (), {})
112-126: Sort all (RUF022) and keep it stable.Minor style nit, improves diffs and tooling friendliness.
Apply this diff:
__all__ = [ - # common - "DataFrame", - "Series", - "Index", - "BoolSeries", - "Mask", - "AgentMask", - "IdsLike", - "ArrayLike", - "KeyBy", - # selectors - "AbstractAgentSetSelector", - "AgentSetSelector", + # common + "AgentMask", + "ArrayLike", + "BoolSeries", + "DataFrame", + "IdysLike", # kept spelling per codebase, but see next comment about 'Continuous' + "Index", + "KeyBy", + "Mask", + "Series", + # selectors + "AbstractAgentSetSelector", + "AgentSetSelector", ]Note: If you prefer to keep a specific grouping, at least alphabetize within groups.
138-153: Spelling: “Continous” → “Continuous” for coordinate aliases.**Typo appears across type aliases (Coordinate/Coordinates). Consider adding new “Continuous*” aliases and keeping the old names as deprecated aliases to avoid churn.
Example:
-ContinousCoordinate = float | Sequence[float] | DataFrame +ContinuousCoordinate = float | Sequence[float] | DataFrame +ContinousCoordinate = ContinuousCoordinate # backward-compat alias -ContinousCoordinates = ( - ContinousCoordinate +ContinuousCoordinates = ( + ContinuousCoordinate | Sequence[float | Sequence[float]] - | Collection[ContinousCoordinate] + | Collection[ContinuousCoordinate] ) +ContinousCoordinates = ContinuousCoordinates # backward-compat aliasmesa_frames/concrete/agentset.py (5)
94-97: Docstring claims snake_case conversion, but code doesn’t implement it.Either implement the normalization or remove the statement to avoid misleading users.
Option A (implement):
- Will be converted to snake_case if in camelCase. + Will be converted to snake_case if in camelCase.Add simple normalizer:
@@ - self._name = name if name is not None else self.__class__.__name__ + proposed = name if name is not None else self.__class__.__name__ + self._name = self._to_snake(proposed)and helper:
@@ class AgentSet(AbstractAgentSet, PolarsMixin): @@ + @staticmethod + def _to_snake(s: str) -> str: + return ( + pl.Series([s]) + .str.to_lowercase() + .str.replace_all(r"(?<!^)([A-Z])", r"_$1") + .str.replace_all(r"[^a-z0-9_]+", "_") + .str.replace_all(r"_+", "_") + .str.strip_chars("_") + .item() + )Option B (remove the claim): delete the sentence in the docstring.
131-149: Avoid bare excepts; scope failures precisely in rename().Catching Exception obscures real bugs. Limit to expected errors and preserve tracebacks where appropriate.
Apply this diff:
- try: + try: if self in self.model.sets: @@ - except Exception: + except (AttributeError, KeyError, ValueError): # Fall back to local rename if delegation fails obj._name = new_name return obj
447-448: Use boolean ‘and’ instead of bitwise ‘&’.Bitwise ops on bools work but are non-idiomatic and can surprise readers.
- if duplicates_allowed & keep_first_only: + if duplicates_allowed and keep_first_only:
681-689: Route name assignment through rename() when attached to a registry.Directly setting _name can violate registry-level uniqueness guarantees. Delegate when possible.
@name.setter def name(self, value: str) -> None: - """Set the name of the AgentSet.""" - self._name = value + """Set the name of the AgentSet, delegating to the registry if present.""" + try: + if self in self.model.sets: + self.rename(value, inplace=True) + return + except Exception: + pass + self._name = value
595-600: Super getattr call is unused.You call super().getattr(key) but discard its result. If the intention is only to trigger the debug guard, add a brief comment. Otherwise, remove the call.
- super().__getattr__(key) + # Trigger AbstractAgentSet debug guard for '_df' access; ignore return. + super().__getattr__(key) return self._df[key]mesa_frames/concrete/agentsetregistry.py (3)
149-160: Drop unused variable ‘single’.Minor cleanup; keeps lints quiet.
- pairs_idx: list[tuple[int, str]] = [(_resolve_one(target), new_name)] - single = True + pairs_idx: list[tuple[int, str]] = [(_resolve_one(target), new_name)] @@ - pairs_idx = [(_resolve_one(k), v) for k, v in target.items()] - single = False + pairs_idx = [(_resolve_one(k), v) for k, v in target.items()] @@ - pairs_idx = [(_resolve_one(k), v) for k, v in target] - single = False + pairs_idx = [(_resolve_one(k), v) for k, v in target]
191-199: Use zip(..., strict=True) to avoid silent truncation.Ensures pairs_idx and target_sets lengths match.
- for aset, (_idx, desired) in zip(target_sets, pairs_idx): + for aset, (_idx, desired) in zip(target_sets, pairs_idx, strict=True):
246-256: Prefer raising from None for not-found lookups.Produces cleaner tracebacks and satisfies B904.
- except KeyError: - raise KeyError(f"Agent set '{name}' not found") + except KeyError: + raise KeyError(f"Agent set '{name}' not found") from NoneApply similarly to other not-found cases in this file.
mesa_frames/abstract/agentset.py (2)
49-52: ClassVar for mutable class attributes (RUF012).mark _copy_only_reference as ClassVar to avoid accidental instance mutation.
-from typing import Any, Literal, Self, overload +from typing import Any, Literal, Self, overload, ClassVar @@ - _copy_only_reference: list[str] = ["_model"] + _copy_only_reference: ClassVar[list[str]] = ["_model"]
59-82: Return docstrings for add()/operators should reflect inplace semantics.Text still says “A new AbstractAgentSet” even when inplace=True. Tweak wording to avoid confusion.
Proposed wording: “The updated AgentSet (or a new copy when inplace=False).”
mesa_frames/abstract/agentsetregistry.py (2)
528-559: Avoid bare except; restrict setitem fallback path.Catching Exception hides real issues. Limit to expected rename failures.
- try: - value.rename(key, inplace=True) - except Exception: + try: + value.rename(key, inplace=True) + except (ValueError, KeyError, TypeError): if hasattr(value, "_name"): value._name = key # type: ignore[attr-defined]Also consider raising from None in explicit KeyErrors elsewhere to satisfy B904.
590-606: keys()/items(): validate key_by via Literal and fail early.You already check at runtime; consider narrowing annotation or centralizing the check to reduce duplication. Non-blocking.
mesa_frames/abstract/space.py (4)
55-55: Remove unused import.
castis imported but not used.-from typing import Any, Literal, Self, cast +from typing import Any, Literal, Self
993-1001: Sample without replacement when targeting “empty/available” cells.With replacement may pick the same cell multiple times, violating intent.
- cells = self.sample_cells(len(agents), cell_type=cell_type) + cells = self.sample_cells( + len(agents), cell_type=cell_type, with_replacement=False + )
1780-1811: Small clean-up: drop redundant Series recast.
agentsis already a Series from_get_ids_srs.- agents = pl.Series(agents) if agents.n_unique() != len(agents): raise ValueError("Some agents are present multiple times")
1362-1370: Parameter doc/validation: radius type check mentions ArrayLike but code paths assume integer radii.Consider tightening docs/types to int | Sequence[int] for grids, or explicitly handle floats.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
docs/general/user-guide/1_classes.md(3 hunks)docs/general/user-guide/2_introductory-tutorial.ipynb(1 hunks)docs/general/user-guide/4_datacollector.ipynb(17 hunks)examples/sugarscape_ig/ss_polars/agents.py(1 hunks)examples/sugarscape_ig/ss_polars/model.py(1 hunks)mesa_frames/abstract/agentset.py(10 hunks)mesa_frames/abstract/agentsetregistry.py(11 hunks)mesa_frames/abstract/datacollector.py(1 hunks)mesa_frames/abstract/space.py(44 hunks)mesa_frames/concrete/agentset.py(6 hunks)mesa_frames/concrete/agentsetregistry.py(6 hunks)mesa_frames/concrete/datacollector.py(3 hunks)mesa_frames/concrete/model.py(2 hunks)mesa_frames/types_.py(2 hunks)tests/conftest.py(1 hunks)tests/test_agents.py(0 hunks)tests/test_agentset.py(1 hunks)tests/test_agentsetregistry.py(1 hunks)tests/test_datacollector.py(8 hunks)tests/test_grid.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/test_agents.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-29T09:25:34.183Z
Learnt from: adamamer20
PR: projectmesa/mesa-frames#143
File: mesa_frames/abstract/space.py:50-63
Timestamp: 2025-04-29T09:25:34.183Z
Learning: The project mesa-frames has been upgraded to Python 3.11, which provides native support for `Self` type in the standard typing module, eliminating the need for imports from typing_extensions.
Applied to files:
mesa_frames/types_.py
🪛 Ruff (0.12.2)
tests/test_agentsetregistry.py
313-313: Prefer next(iter(reg.values())) over single element slice
Replace with next(iter(reg.values()))
(RUF015)
mesa_frames/concrete/datacollector.py
188-188: Do not catch blind exception: Exception
(BLE001)
239-241: Abstract raise to an inner function
(TRY301)
239-241: Avoid specifying long messages outside the exception class
(TRY003)
243-243: Do not catch blind exception: Exception
(BLE001)
258-260: Avoid specifying long messages outside the exception class
(TRY003)
264-266: Avoid specifying long messages outside the exception class
(TRY003)
555-555: Do not catch blind exception: Exception
(BLE001)
mesa_frames/concrete/agentsetregistry.py
88-90: Avoid specifying long messages outside the exception class
(TRY003)
142-142: Avoid specifying long messages outside the exception class
(TRY003)
147-147: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Avoid specifying long messages outside the exception class
(TRY003)
159-159: Local variable single is assigned to but never used
Remove assignment to unused variable single
(F841)
191-191: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
248-248: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
248-248: Avoid specifying long messages outside the exception class
(TRY003)
256-256: Avoid specifying long messages outside the exception class
(TRY003)
265-265: Avoid specifying long messages outside the exception class
(TRY003)
267-269: Avoid specifying long messages outside the exception class
(TRY003)
275-277: Avoid specifying long messages outside the exception class
(TRY003)
281-283: Avoid specifying long messages outside the exception class
(TRY003)
291-293: Avoid specifying long messages outside the exception class
(TRY003)
304-306: Avoid specifying long messages outside the exception class
(TRY003)
309-309: Avoid specifying long messages outside the exception class
(TRY003)
385-385: Avoid specifying long messages outside the exception class
(TRY003)
612-612: Avoid specifying long messages outside the exception class
(TRY003)
654-656: Avoid specifying long messages outside the exception class
(TRY003)
697-697: Avoid specifying long messages outside the exception class
(TRY003)
700-700: Avoid specifying long messages outside the exception class
(TRY003)
mesa_frames/abstract/agentsetregistry.py
102-102: Undefined name mesa_frames
(F821)
104-104: Undefined name mesa_frames
(F821)
105-105: Undefined name mesa_frames
(F821)
146-146: Undefined name mesa_frames
(F821)
147-147: Undefined name mesa_frames
(F821)
172-172: Undefined name mesa_frames
(F821)
173-173: Undefined name mesa_frames
(F821)
183-183: Undefined name mesa_frames
(F821)
184-184: Undefined name mesa_frames
(F821)
233-233: Undefined name mesa_frames
(F821)
251-251: Undefined name mesa_frames
(F821)
287-287: Undefined name mesa_frames
(F821)
293-293: Undefined name mesa_frames
(F821)
299-299: Undefined name mesa_frames
(F821)
301-301: Undefined name mesa_frames
(F821)
307-307: Undefined name mesa_frames
(F821)
308-308: Undefined name mesa_frames
(F821)
309-309: Undefined name mesa_frames
(F821)
312-312: Undefined name mesa_frames
(F821)
313-313: Undefined name mesa_frames
(F821)
320-320: Undefined name mesa_frames
(F821)
321-321: Undefined name mesa_frames
(F821)
322-322: Undefined name mesa_frames
(F821)
325-325: Undefined name mesa_frames
(F821)
326-326: Undefined name mesa_frames
(F821)
359-359: Undefined name mesa_frames
(F821)
360-360: Undefined name mesa_frames
(F821)
430-430: Undefined name mesa_frames
(F821)
431-431: Undefined name mesa_frames
(F821)
437-437: Undefined name mesa_frames
(F821)
445-445: Undefined name mesa_frames
(F821)
450-450: Undefined name mesa_frames
(F821)
454-454: Undefined name mesa_frames
(F821)
455-455: Undefined name mesa_frames
(F821)
458-458: Undefined name mesa_frames
(F821)
460-460: Undefined name mesa_frames
(F821)
461-461: Undefined name mesa_frames
(F821)
468-468: Undefined name mesa_frames
(F821)
469-469: Undefined name mesa_frames
(F821)
489-489: Undefined name mesa_frames
(F821)
490-490: Undefined name mesa_frames
(F821)
510-510: Undefined name mesa_frames
(F821)
511-511: Undefined name mesa_frames
(F821)
531-531: Undefined name mesa_frames
(F821)
542-542: Avoid specifying long messages outside the exception class
(TRY003)
554-554: Do not catch blind exception: Exception
(BLE001)
559-559: Avoid specifying long messages outside the exception class
(TRY003)
566-566: Undefined name mesa_frames
(F821)
592-592: Undefined name mesa_frames
(F821)
602-602: Avoid specifying long messages outside the exception class
(TRY003)
611-611: Undefined name mesa_frames
(F821)
612-612: Undefined name mesa_frames
(F821)
625-625: Avoid specifying long messages outside the exception class
(TRY003)
630-630: Undefined name mesa_frames
(F821)
mesa_frames/types_.py
112-126: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
mesa_frames/abstract/space.py
237-237: Avoid specifying long messages outside the exception class
(TRY003)
239-239: Avoid specifying long messages outside the exception class
(TRY003)
241-241: Avoid specifying long messages outside the exception class
(TRY003)
243-243: Avoid specifying long messages outside the exception class
(TRY003)
mesa_frames/abstract/agentset.py
49-49: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
460-460: Undefined name mesa_frames
(F821)
468-468: Undefined name mesa_frames
(F821)
mesa_frames/concrete/agentset.py
86-86: Undefined name mesa_frames
(F821)
138-138: Do not catch blind exception: Exception
(BLE001)
146-146: Do not catch blind exception: Exception
(BLE001)
326-326: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (7)
tests/test_agentset.py (1)
263-292: Rename workflow test looks solid.Covers inplace, collision/canonicalization, and copy semantics; exercises registry mapping updates.
If non-inplace rename should or should not auto-register the copy, consider asserting the intended behavior to lock it down.
docs/general/user-guide/1_classes.md (2)
46-48: LGTM: registry-based lookup example is clear.
95-97: LGTM: step delegation via registry.mesa_frames/concrete/model.py (2)
129-131: LGTM:step()dispatches via registry.do("step").
89-91: Confirm step-wrapping captures subclass overrides.
self._user_step = self.stepmust bind the subclass’s override whensuper().__init__()runs. If any subclass rebindsself.stepaftersuper().__init__(), wrapping will be bypassed. Worth a quick audit in the codebase.tests/test_datacollector.py (1)
167-167: LGTM: agent reporter callables now receive the registry (sets).This aligns with the documented semantics and exercises registry-level access cleanly.
Also applies to: 226-226, 282-282, 364-365, 440-441, 516-517, 565-566, 710-711
mesa_frames/abstract/agentsetregistry.py (1)
664-675: ids property docstring OK; ensure concrete returns pl.Series consistently.Looks good; verify concrete implementation dtype is pl.UInt64 (it is).
If you want me to add a quick assertion-based test for ids dtype and emptiness semantics, I can draft it.
adamamer20
changed the title
AgentSetDF and AgentSetsAccessor for name/type/index lookup and container-aware keying
adamamer20
changed the title
AgentSets + flexible lookup & stricter separation of concerns
adamamer20
changed the title
AgentSets + flexible lookup & stricter separation of concernsAgentSet + flexible lookup & stricter separation of concerns
This PR simplifies the mental model and usage of
AgentSet/AgentSetRegistryby:pl.DataFrame.This yields a friendlier, clearer and safer API for everyday tasks (query one set, broadcast across sets).
What changed
1) Registry scope
AgentSetRegistry.dfreturned a mapping{AgentSet → pl.DataFrame}and high-level helpers likegetoften operated directly on the sets’ underlying DataFrames, which was easy to misread as “registry-wide” operations.dfinterface anymore, nor any “agents” iterator. You always work per set (viamodel.sets[...]) and then use that set’s API (.df,.get,.set, …). The registry orchestrates and routes, but never acts like a DataFrame holder.Before → After: accessing DataFrames
Get one set’s frame
Iterate multiple sets’ frames
2) Named sets & flexible lookup
Summary by CodeRabbit
New Features
Documentation
Refactor