Skip to content

Generate automatically unique_id #110

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

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

luca-patrignani
Copy link

@luca-patrignani luca-patrignani commented Mar 4, 2025

This PR aims to address issue #105 by generating automatically the unique_id for each agent and not letting the user assign it.
The proposed solution consists in assigning increasing ids to new added agents.

Need confirmation

  • When adding a new agent with a user-defined id the add method will throw a warning and ignore the id

Summary by CodeRabbit

  • New Features

    • Introduced dynamic unique identifier generation that supports a broader range of agent IDs and flexible agent additions.
  • Refactor

    • Streamlined internal handling of agent identifiers for more consistent agent addition, removal, and validation.
    • Enhanced data type management across integrations for improved reliability.
  • Tests

    • Updated testing procedures to ensure dynamic ID handling and robust agent management throughout typical workflows.
  • Documentation

    • Refined internal documentation to clarify usage of updated agent management interfaces.

@luca-patrignani luca-patrignani marked this pull request as ready for review March 6, 2025 20:54
@luca-patrignani
Copy link
Author

@adamamer20
So I would you deal with case when I add a new agent set and their ids are already present? My current solution consists on reindexing but this might be wrong. What about leaving the responsibility of keeping the ids unique to the user?

Copy link
Member

@adamamer20 adamamer20 left a comment

Choose a reason for hiding this comment

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

added exception for adding unique id

@adamamer20
Copy link
Member

@adamamer20 So I would you deal with case when I add a new agent set and their ids are already present? My current solution consists on reindexing but this might be wrong. What about leaving the responsibility of keeping the ids unique to the user?

Since we handle ids internally, I think the burden of keeping them unique is on us.

@luca-patrignani
Copy link
Author

@adamamer20 Right now polars backend passes all tests, the pandas one still needs some fixes, mainly adapting the current unit tests for using automatically generated unique ids.

Things that need confirmation

  • When all agents are active self._mask is an empty series and it is filled when as soon as a select is invoked
  • Unit tests have been changed a lot, they surely need some double-check

Copy link
Member

@adamamer20 adamamer20 left a comment

Choose a reason for hiding this comment

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

i think the code is well written. delete the changes for pandas, and make that change in id generation.

Copy link
Contributor

coderabbitai bot commented Apr 1, 2025

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This update enhances agent ID handling across the codebase. The changes switch several internal agent ID types to unsigned 64-bit integers (uint64) and adjust method signatures and DataFrame constructions accordingly. New unique ID generation methods have been added for both Pandas and Polars implementations. Minor adjustments in mixin classes (such as docstring formatting) and changes in tests to dynamically reference agent IDs further streamline the functionality. Overall, these modifications standardize and improve the robustness of agent data management.

Changes

File(s) Change Summary
mesa_frames/abstract/space.py Added pandas import and introduced AgnosticIds; changed GridDF's agent_id datatype from int to uint64.
mesa_frames/concrete/agents.py Refactored AgentsDF to use pl.UInt64 for _ids; updated add, contains, remove, and ID presence checks to accommodate unsigned integer types.
mesa_frames/concrete/pandas/agentset.py
mesa_frames/concrete/pandas/mixin.py
Updated AgentSetPandas to use uint64 for unique IDs; enhanced the add, contains, and set methods (with added _generate_unique_ids); refined mixin with docstring and series improvements.
mesa_frames/concrete/polars/agentset.py
mesa_frames/concrete/polars/mixin.py
Enhanced AgentSetPolars with stricter unique ID validation, added _generate_unique_ids, improved mask and error handling; expanded Polars type mapping in the mixin.
tests/pandas/test_agentset_pandas.py
tests/pandas/test_grid_pandas.py
Streamlined ExampleAgentSetPandas (removed the index parameter) and introduced a new get_unique_ids function in grid tests; updated assertions to use dynamic ID references.
tests/polars/test_agentset_polars.py
tests/polars/test_grid_polars.py
Modified Polars tests to eliminate direct unique_id handling; integrated get_unique_ids calls and revised assertions in add, discard, remove, and select for robust error handling.
tests/test_agents.py Revised agent tests by removing duplicate error checks and updating assertions (using to_list() and reindexing) to consistently access agent identifiers.

Sequence Diagram(s)

sequenceDiagram
    participant C as Client
    participant A as AgentSet (Pandas/Polars)
    participant U as _generate_unique_ids
    participant DF as DataFrame

    C->>A: Call add(agents)
    alt Missing unique IDs
        A->>U: Generate unique IDs
        U-->>A: Return new IDs
    end
    A->>DF: Update agents with uint64 IDs
    DF-->>A: Updated DataFrame
    A-->>C: Return updated AgentSet
Loading

Poem

I'm a little rabbit, hopping with cheer,
Agent IDs now sparkle, perfectly clear.
UInt64 magic, in every byte so bright,
New methods hop in, making data just right.
With every line of code, I dance in delight,
Celebrating smooth changes from morning to night!
🐇✨


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@luca-patrignani
Copy link
Author

luca-patrignani commented Apr 1, 2025

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🔭 Outside diff range comments (1)
tests/pandas/test_grid_pandas.py (1)

700-700: 💡 Verification agent

🧩 Analysis chain

Fix agent ID validation in test methods.

The pipeline failures indicate "ValueError: Some agents are not in the model" in several test methods. Ensure that agent IDs used in these methods are correctly retrieved using get_unique_ids() and that you're using valid indices.

Also applies to: 744-744, 888-888, 932-932


🏁 Script executed:

#!/bin/bash
# Find all occurrences of methods that might be causing the errors
rg -A 2 "test_move_to_available|test_move_to_empty|test_place_to_available|test_place_to_empty" tests/pandas/test_grid_pandas.py

Length of output: 601


Fix agent ID validation in test methods

The pipeline failures (“ValueError: Some agents are not in the model”) suggest that the tests are using hard-coded or invalid agent IDs (e.g. a literal “0”) rather than dynamically retrieving them. Please update the following test methods in tests/pandas/test_grid_pandas.py so that they obtain valid agent IDs via grid_moore.get_unique_ids() before using them:

  • test_move_to_available (line 700)
  • test_move_to_empty (line 744)
  • test_place_to_available (line 888)
  • test_place_to_empty (line 932)

Once revised, re-run the test suite to ensure that the “Some agents are not in the model” error no longer occurs.

🧰 Tools
🪛 GitHub Actions: build

[error] 700-700: ValueError: Some agents are not in the model

🧹 Nitpick comments (11)
tests/pandas/test_grid_pandas.py (1)

8-8: Unused import "AgentSetDF".

The import mesa_frames.abstract.agents.AgentSetDF is not used in the file.

-from mesa_frames.abstract.agents import AgentSetDF
🧰 Tools
🪛 Ruff (0.8.2)

8-8: mesa_frames.abstract.agents.AgentSetDF imported but unused

Remove unused import: mesa_frames.abstract.agents.AgentSetDF

(F401)

tests/polars/test_agentset_polars.py (1)

100-107: Verify comparison method in test assertions.

When comparing Series objects, use of all() may not guarantee proper element-wise comparison. Consider using more robust comparison methods like pl.Series.equals() or converting to lists before comparison.

-assert all(
-    result["unique_id"]
-    == pl.concat([agents["unique_id"], agents2["unique_id"]])
-)
+assert result["unique_id"].equals(
+    pl.concat([agents["unique_id"], agents2["unique_id"]])
+)

Also applies to: 316-321

mesa_frames/abstract/space.py (2)

57-57: Remove unused import pandas

The pandas module is imported but not used in this file. This is confirmed by the static analyzer which flagged it as an unused import (F401).

-import pandas as pd
🧰 Tools
🪛 Ruff (0.8.2)

57-57: pandas imported but unused

Remove unused import: pandas

(F401)


66-66: Remove unused import AgnosticIds

The AgnosticIds type is imported from mesa_frames.types_ but not used in this file. Static analysis confirms this as an unused import (F401).

-    AgnosticIds,
🧰 Tools
🪛 Ruff (0.8.2)

66-66: mesa_frames.types_.AgnosticIds imported but unused

Remove unused import: mesa_frames.types_.AgnosticIds

(F401)

tests/test_agents.py (1)

214-218: Add a comment explaining the reindexing

The code now saves the original index order and reindexes after the operation. This is good practice to ensure test stability, but it would be helpful to add a comment explaining why this is necessary.

 original_index_for_pandas_df = agents._agentsets[0].index.copy()
 agents.do("add_wealth", 1, mask=mask_dictionary)
+# Reindex to ensure consistent ordering for test assertions, as do() method may change order
 agents._agentsets[0].agents = agents._agentsets[0].agents.reindex(
     original_index_for_pandas_df
 )
mesa_frames/concrete/pandas/agentset.py (1)

121-121: Consider rewriting Yoda condition.
The line if "unique_id" == agents.index.name or ... might read more naturally as if agents.index.name == "unique_id".

- if "unique_id" == agents.index.name or "unique_id" in agents.columns:
+ if agents.index.name == "unique_id" or "unique_id" in agents.columns:
🧰 Tools
🪛 Ruff (0.8.2)

121-121: Yoda condition detected

Rewrite as agents.index.name == "unique_id"

(SIM300)

mesa_frames/concrete/polars/agentset.py (2)

62-63: Remove unused imports.
The uuid and warnings imports are not referenced in this file.

- import uuid
- import warnings
🧰 Tools
🪛 Ruff (0.8.2)

62-62: uuid imported but unused

Remove unused import: uuid

(F401)


63-63: warnings imported but unused

Remove unused import: warnings

(F401)


220-220: Clarify exception chaining.
Suggest using raise KeyError(error) from error to preserve the original traceback.

- raise KeyError(error)
+ raise KeyError(error) from error
🧰 Tools
🪛 Ruff (0.8.2)

220-220: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

tests/polars/test_grid_polars.py (3)

34-35: Commented code should be removed

The commented-out sorting operations are no longer needed with the automatic unique ID generation system, but they should be removed rather than left as comments.

-        # fix1_AgentSetPandas.agents = fix1_AgentSetPandas.agents.sort_index(inplace=False)
-        # fix2_AgentSetPolars.agents = fix2_AgentSetPolars.agents.sort("unique_id")

975-978: Simplify nested conditional

This nested if statement can be simplified.

-            if last is not None and not different:
-                if (
-                    space.agents.select(pl.col("dim_0", "dim_1")).to_numpy() != last
-                ).any():
-                    different = True
+            if last is not None and not different and (
+                space.agents.select(pl.col("dim_0", "dim_1")).to_numpy() != last
+            ).any():
+                different = True
🧰 Tools
🪛 Ruff (0.8.2)

975-978: Use a single if statement instead of nested if statements

Combine if statements using and

(SIM102)


1312-1315: Simplify nested conditional

Similar to the earlier case, this nested if statement can be simplified.

-            if last is not None and not different:
-                if (
-                    space.agents.select(pl.col("dim_0", "dim_1")).to_numpy() != last
-                ).any():
-                    different = True
+            if last is not None and not different and (
+                space.agents.select(pl.col("dim_0", "dim_1")).to_numpy() != last
+            ).any():
+                different = True
🧰 Tools
🪛 Ruff (0.8.2)

1312-1315: Use a single if statement instead of nested if statements

Combine if statements using and

(SIM102)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c2022dc and 9c1722a.

📒 Files selected for processing (11)
  • mesa_frames/abstract/space.py (3 hunks)
  • mesa_frames/concrete/agents.py (7 hunks)
  • mesa_frames/concrete/pandas/agentset.py (9 hunks)
  • mesa_frames/concrete/pandas/mixin.py (1 hunks)
  • mesa_frames/concrete/polars/agentset.py (9 hunks)
  • mesa_frames/concrete/polars/mixin.py (2 hunks)
  • tests/pandas/test_agentset_pandas.py (15 hunks)
  • tests/pandas/test_grid_pandas.py (27 hunks)
  • tests/polars/test_agentset_polars.py (13 hunks)
  • tests/polars/test_grid_polars.py (32 hunks)
  • tests/test_agents.py (12 hunks)
🧰 Additional context used
🧬 Code Definitions (7)
tests/pandas/test_grid_pandas.py (2)
mesa_frames/concrete/pandas/space.py (1)
  • remaining_capacity (235-238)
mesa_frames/concrete/pandas/agentset.py (4)
  • agents (434-435)
  • agents (438-445)
  • pos (464-465)
  • index (460-461)
tests/polars/test_agentset_polars.py (1)
mesa_frames/concrete/polars/agentset.py (11)
  • agents (541-542)
  • agents (545-548)
  • add (105-161)
  • contains (164-164)
  • contains (167-167)
  • contains (169-178)
  • active_agents (551-552)
  • active_agents (555-556)
  • select (253-272)
  • AgentSetPolars (83-568)
  • inactive_agents (559-560)
tests/test_agents.py (1)
mesa_frames/concrete/pandas/agentset.py (6)
  • agents (434-435)
  • agents (438-445)
  • contains (155-155)
  • contains (158-158)
  • contains (160-170)
  • index (460-461)
tests/pandas/test_agentset_pandas.py (1)
mesa_frames/concrete/pandas/agentset.py (13)
  • agents (434-435)
  • agents (438-445)
  • pos (464-465)
  • index (460-461)
  • add (112-152)
  • contains (155-155)
  • contains (158-158)
  • contains (160-170)
  • active_agents (448-449)
  • active_agents (452-453)
  • select (230-250)
  • AgentSetPandas (76-465)
  • inactive_agents (456-457)
mesa_frames/concrete/pandas/agentset.py (4)
mesa_frames/concrete/polars/agentset.py (4)
  • add (105-161)
  • agents (541-542)
  • agents (545-548)
  • _generate_unique_ids (485-493)
mesa_frames/concrete/agents.py (3)
  • add (88-117)
  • agents (553-554)
  • agents (557-565)
mesa_frames/abstract/agents.py (8)
  • add (104-123)
  • add (746-770)
  • agents (651-657)
  • agents (661-667)
  • agents (1043-1044)
  • agents (1047-1055)
  • AgentSetDF (727-1075)
  • random (630-637)
mesa_frames/concrete/model.py (2)
  • agents (162-180)
  • agents (183-186)
mesa_frames/concrete/agents.py (2)
mesa_frames/concrete/polars/agentset.py (3)
  • agents (541-542)
  • agents (545-548)
  • index (563-564)
mesa_frames/concrete/pandas/agentset.py (3)
  • agents (434-435)
  • agents (438-445)
  • index (460-461)
tests/polars/test_grid_polars.py (1)
tests/pandas/test_grid_pandas.py (6)
  • get_unique_ids (19-24)
  • model (36-43)
  • grid_moore (46-53)
  • grid_hexagonal (73-77)
  • grid_moore_torus (56-63)
  • grid_von_neumann (66-70)
🪛 Ruff (0.8.2)
tests/pandas/test_grid_pandas.py

8-8: mesa_frames.abstract.agents.AgentSetDF imported but unused

Remove unused import: mesa_frames.abstract.agents.AgentSetDF

(F401)

mesa_frames/abstract/space.py

57-57: pandas imported but unused

Remove unused import: pandas

(F401)


66-66: mesa_frames.types_.AgnosticIds imported but unused

Remove unused import: mesa_frames.types_.AgnosticIds

(F401)

mesa_frames/concrete/pandas/agentset.py

121-121: Yoda condition detected

Rewrite as agents.index.name == "unique_id"

(SIM300)

mesa_frames/concrete/polars/agentset.py

62-62: uuid imported but unused

Remove unused import: uuid

(F401)


63-63: warnings imported but unused

Remove unused import: warnings

(F401)


220-220: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

tests/polars/test_grid_polars.py

975-978: Use a single if statement instead of nested if statements

Combine if statements using and

(SIM102)


1312-1315: Use a single if statement instead of nested if statements

Combine if statements using and

(SIM102)

🪛 GitHub Actions: build
tests/pandas/test_grid_pandas.py

[error] 700-700: ValueError: Some agents are not in the model


[error] 744-744: ValueError: Some agents are not in the model


[error] 812-812: AssertionError


[error] 888-888: ValueError: Some agents are not in the model


[error] 932-932: ValueError: Some agents are not in the model


[error] 1000-1000: ValueError: Some agents are not present in the model


[error] 1199-1199: ValueError: Some agents are not present in the model


[error] 1249-1249: AssertionError


[error] 1277-1277: AssertionError


[error] 1296-1296: AssertionError

🔇 Additional comments (59)
mesa_frames/concrete/pandas/mixin.py (1)

54-56: Formatting improvement in docstring.

The removal of the # noqa: D205 directive suggests that the docstring now complies with style guidelines without needing to suppress linting warnings.

tests/pandas/test_grid_pandas.py (2)

3-3: Added import for Polars library.

This import supports the new get_unique_ids function that uses Polars Series concatenation.


19-25: New helper function to retrieve unique agent IDs.

The get_unique_ids function aggregates unique IDs from different agent types in the model, supporting the automatic ID generation feature. This function is used throughout the test file to replace hardcoded agent IDs.

tests/polars/test_agentset_polars.py (2)

72-91: Good test coverage for rejecting user-defined unique IDs.

The new test test_add_with_unique_id properly verifies that the system rejects attempts to add agents with predefined IDs, which is a key requirement for the automatic ID generation feature. The test covers all input types: DataFrame, list, and dictionary.


44-44: Good adaptation of tests to use dynamic agent IDs.

The code now properly uses agents["unique_id"] with array indexing to select specific agents, replacing hardcoded ID references. This approach aligns with the automatic ID generation feature.

Also applies to: 54-56

mesa_frames/concrete/polars/mixin.py (2)

61-65: Added support for uint64 data type.

The addition of "uint64" mapping to pl.UInt64 in the _dtypes_mapping dictionary enhances type handling capabilities for agent IDs, which are now stored as unsigned 64-bit integers. This change aligns with the automatic ID generation feature.


615-615: Improved robustness of series constructor.

Converting data to a list before passing it to the Polars Series constructor ensures consistent behavior regardless of the iterable type provided. This makes the method more robust and prevents potential issues with different iterable types.

mesa_frames/abstract/space.py (1)

1174-1174: Good change: Using uint64 for agent IDs

Changing the agent_id data type to uint64 aligns with the PR objective of automating unique ID generation. Unsigned integers are appropriate for IDs since they only need non-negative values, and using 64 bits provides a large range (0 to 2^64-1) for potential IDs.

tests/test_agents.py (4)

99-105: Good update: Using dynamic references to agent IDs

The test now references agent IDs dynamically through agentset_pandas["unique_id"][0] instead of hardcoded values. This makes the test more robust when agent IDs are auto-generated rather than user-defined.


159-160: Update to support autogenerated IDs

The assertion now correctly checks that ID 0 is not in the agents collection, which aligns with the PR's goal of automatically generating unique IDs rather than using sequential integers starting from 0.


270-271: Consider updating comment to reflect changes

This comment refers to converting to a Python list, which is now necessary because both Pandas and Polars implementations need to access the first element consistently.


323-325: Consistent with autogenerated ID approach

The test now correctly checks that ID 0 is not in the agents collection and generates an appropriate exception when trying to remove it. This aligns with the PR's goal of automatically generating unique IDs.

tests/pandas/test_agentset_pandas.py (12)

14-16: Good simplification: Removed index parameter

Removing the index parameter from the constructor aligns with the PR's goal of automating unique ID generation. The simplification of starting_wealth initialization is appropriate as IDs will now be generated automatically.


28-28: Updated initialization to support autogenerated IDs

The constructor call for ExampleAgentSetPandas has been updated to remove the index parameter, which is consistent with the PR's goal of automating ID generation.


38-38: Updated initialization consistent with first fixture

Good consistency - both fixtures have been updated to use the new constructor signature without an index parameter.


49-51: Using unique_id attribute for agent references

The code now correctly references agent IDs through fix1_AgentSetPandas["unique_id"][:2] instead of using hardcoded indices, making the test more robust.


66-78: Good test: Validating that manual ID setting is prevented

This new test verifies that the system properly rejects attempts to add agents with predefined IDs, which aligns with the PR objective of automating unique ID generation. The test checks both list and dictionary inputs.


89-91: Simplified test assertions

The test now focuses on verifying the length of the agent collection after adding without checking specific ID values, which is appropriate for the autogenerated ID approach.


94-96: Simplified test assertions

Similar to the previous change, this test now focuses on verifying the length without depending on specific ID values, which is appropriate for autogenerated IDs.


101-102: Simplified dictionary input for add method

The test now adds agents using a simpler dictionary without specifying unique_id values, which aligns with the PR's goal of automating ID generation.


111-112: Good update: Using dynamic references to agent IDs

The test now references agent IDs dynamically through agents["unique_id"][0] instead of hardcoded values, making it more robust with autogenerated IDs.


115-115: Using dynamic references for agent IDs

Continuing the pattern of using dynamic ID references rather than hardcoded values, which supports the PR's objective.


183-187: Improved explanation and reindexing

Added a helpful comment explaining that the do method doesn't guarantee order preservation and correctly reindexes the result to ensure consistent ordering for test assertions.


296-303: Updated test for agent concatenation

The test now correctly verifies that when adding two agent sets, the resulting agent set contains IDs from both source sets, using dynamic ID references rather than hardcoded values.

mesa_frames/concrete/pandas/agentset.py (7)

56-56: Import usage is valid.
The warnings import is indeed used to emit a deprecation warning, so there's no issue here.


114-141: Add method signature update and unique ID checks look correct.
Adding AgentSetDF to the accepted types aligns with the PR objective, and preventing user-defined unique_id columns or keys helps maintain automatically generated IDs. The condition is logically sound.

🧰 Tools
🪛 Ruff (0.8.2)

121-121: Yoda condition detected

Rewrite as agents.index.name == "unique_id"

(SIM300)


146-148: Verify necessity of handling Polars DataFrame in a Pandas class.
Although checking for a Polars DataFrame is not harmful, it's unclear if it's ever expected here, given the function signature. Consider removing or explaining this branch if it’s not truly needed.


152-159: Mixed usage of Polars references in a Pandas class.
The logic referencing pl.Series for _mask and originally_empty may be bridging code, but please confirm that it’s intentional and consistent with the design.


213-214: Double-check usage of a Polars DataFrame in set operation.
Lines handle Polars values in a Pandas-based set(...); confirm if passing a Polars DataFrame is a legitimate scenario or an edge case that should be disallowed.


407-415: Uniform unique ID generation aligns with PR goals.
Generating np.uint64 IDs in debug mode ensures collisions are tested. This approach seems solid and consistent with the Polars counterpart.


449-449: Conditional active agents return is reasonable.
Falling back to the whole DataFrame if the mask is empty matches the documented behavior.

mesa_frames/concrete/polars/agentset.py (7)

67-67: ShapeError import is justified.
This import is required for catching column shape issues in the set method.


107-159: Add method modifications: consistent with unique ID enforcement.
Rejecting a user-defined unique_id ensures the PR’s automatic ID assignment strategy. The diagonal relaxation in pl.concat properly combines data.


175-176: Collection-based contain checks.
Using an unsigned integer series for ID checks aligns with the new ID type.


213-214: Accessing values as to_series() is valid.
This approach helps unify the assignment logic regardless of input type.


239-245: Conditional unique_id column injection looks good.
Generating new IDs if missing helps maintain consistency with automated ID logic.


250-250: Updating mask with new indices is correct.
The call to _update_mask ensures the newly added agents are active if needed.


485-493: Polars unique ID generation method is consistent with the Pandas counterpart.
Using large ranges in debug mode for collision testing is a solid approach.

mesa_frames/concrete/agents.py (10)

51-51: Importing NumPy is appropriate.
It’s used to handle np.uint64 checks, aligning with the PR’s automated ID design.


86-86: Switch to UInt64 for _ids.
This is consistent with the PR’s switch to unsigned IDs across the codebase.


114-116: Appending new agent sets and their IDs.
Concatenating each agentset’s unique_id properly extends the global IDs.


128-128: Checking for np.uint64.
Validates the type more precisely than a generic integer check.


141-141: Collection-based ID containment.
Using pl.Series(..., dtype=pl.UInt64).is_in(...) ensures consistent ID comparison.


213-213: Handling single ID removal as (int, np.uint64).
This is a clear improvement that accounts for both regular int and the new uint64.


225-232: Building removed IDs with pl.UInt64.
This ensures the removed set is handled consistently with the rest of the code.


240-241: Extracting and rewrapping IDs as pl.Series (uint64).
This step maintains consistent ID typing during removals.


357-358: Presence DataFrame updated with pl.UInt64 schema.
Ensures that ID presence checks align with the newly introduced ID type.


361-361: Enforcing unsigned type for incoming agentset indexes.
Casting agentset indexes to pl.UInt64 unifies the ID type across the system.

tests/polars/test_grid_polars.py (11)

5-5: LGTM: Added necessary imports for the new unique ID handling

The imports for assert_frame_equal from polars.testing and get_unique_ids from the pandas test module are appropriate for the changes being implemented throughout this file.

Also applies to: 12-12


42-43: LGTM: Good refactoring to use dynamic IDs

Replacing hardcoded agent IDs with dynamic ID retrieval via get_unique_ids makes the tests more robust and consistent with the new unique ID generation system.

Also applies to: 52-53, 62-63, 69-70


145-145: LGTM: Properly updated test assertions to use dynamic IDs

The test assertions have been updated to use dynamically retrieved agent IDs, ensuring that tests remain valid with the new unique ID generation system.

Also applies to: 166-176, 178-180, 204-205, 251-254


638-641: LGTM: Proper agent movement with dynamic IDs

Agent movement operations throughout the test now use dynamically retrieved IDs, maintaining consistency with the new unique ID generation approach.

Also applies to: 732-734, 750-753


645-655: LGTM: Updated assertion checks to use dynamic IDs

The assert_frame_equal checks now properly validate DataFrames containing dynamically generated agent IDs. The implementation maintains test integrity while accommodating the new unique ID system.

Also applies to: 673-679, 683-693, 697-707, 715-725, 736-746, 755-766


811-812: LGTM: Consistent ID usage across agent operations

All agent operations (move, place, remove) have been updated to use dynamic IDs retrieved via get_unique_ids, making the test suite consistent with the automated ID generation feature.

Also applies to: 865-866, 897-898, 909-910, 914-914, 932-932, 969-970, 974-974, 992-992


814-820: LGTM: Updated DataFrame assertions to use dynamic IDs

All DataFrame assertions now correctly compare actual results against expected values using dynamically generated IDs. This ensures tests remain valid with the new automatic ID generation system.

Also applies to: 830-840, 850-860, 883-893, 899-905


1050-1066: LGTM: Updated place_agents tests to use dynamic IDs

The tests for the place_agents functionality have been properly updated to use dynamically generated IDs, maintaining test coverage while accommodating the new ID system.

Also applies to: 1084-1170, 1227-1240


1401-1462: LGTM: Updated remove_agents tests to use dynamic IDs

All agent removal tests now correctly use dynamic IDs from the get_unique_ids function, ensuring proper test functionality with the new ID system.


1588-1589: LGTM: Updated swap_agents tests to use dynamic IDs

The swap_agents tests now correctly retrieve and use dynamic agent IDs, maintaining test functionality with the new unique ID generation system.

Also applies to: 1599-1607, 1608-1643


1671-1701: LGTM: Updated property tests to use dynamic IDs

The property tests for agents and cells have been updated to properly use dynamic IDs, ensuring that all assertions work correctly with the new ID generation system.

Also applies to: 1719-1748

@adamamer20
Copy link
Member

This needs some attention: https://github.com/luca-patrignani/mesa-frames/blob/9c1722a79ba46e4043b08a356c769b9218b204b0/mesa_frames/concrete/polars/agentset.py#L486-L490

Sorry @luca-patrignani can you explain better what you mean? remember that polars has a dtype pl.UInt64 that can be specified explicitly in the constructor.

@luca-patrignani
Copy link
Author

This needs some attention: https://github.com/luca-patrignani/mesa-frames/blob/9c1722a79ba46e4043b08a356c769b9218b204b0/mesa_frames/concrete/polars/agentset.py#L486-L490

Sorry @luca-patrignani can you explain better what you mean? remember that polars has a dtype pl.UInt64 that can be specified explicitly in the constructor.

My bad, that piece is completely useless. I am removing it.

Copy link

codecov bot commented Apr 5, 2025

Codecov Report

Attention: Patch coverage is 88.67925% with 6 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@5078421). Learn more about missing BASE report.

Files with missing lines Patch % Lines
mesa_frames/concrete/agentset.py 89.74% 4 Missing ⚠️
mesa_frames/abstract/space.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #110   +/-   ##
=======================================
  Coverage        ?   92.19%           
=======================================
  Files           ?       11           
  Lines           ?     1692           
  Branches        ?        0           
=======================================
  Hits            ?     1560           
  Misses          ?      132           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@adamamer20 adamamer20 left a comment

Choose a reason for hiding this comment

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

Almost there @luca-patrignani!
This is not an easy task. Check my review comments...

@@ -95,12 +98,12 @@ def __init__(self, model: "ModelDF") -> None:
The model that the agent set belongs to.
"""
self._model = model
self._agents = pl.DataFrame(schema={"unique_id": pl.Int64})
self._agents = pl.DataFrame()
self._mask = pl.repeat(True, len(self._agents), dtype=pl.Boolean, eager=True)

def add(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def add(
agents: pl.DataFrame | Sequence[Any] | dict[str, Any],

Copy link
Member

Choose a reason for hiding this comment

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

You cannot add agents from another AgentSetPolars

@@ -188,7 +196,6 @@ def set(
inplace: bool = True,
) -> Self:
obj = self._get_obj(inplace)
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why you had to make changes to the set method?

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