-
Notifications
You must be signed in to change notification settings - Fork 14
Added automatic generation of unique_ids. fix issue #105 #134
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
Added automatic generation of unique_ids. fix issue #105 #134
Conversation
Make unique_ids optional by auto-generating them when not provided,
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #134 +/- ##
=======================================
Coverage ? 91.30%
=======================================
Files ? 14
Lines ? 2242
Branches ? 0
=======================================
Hits ? 2047
Misses ? 195
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
if len(obj._agents) == 0: | ||
# If no agents exist yet, start from 0 | ||
new_id = 0 | ||
else: | ||
# Otherwise, use max existing ID + 1 | ||
new_id = obj._agents["unique_id"].max() + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is it not better to just use new_id = len(obj._agents)
minor improvement time_complexity will be O(1) instead of O(n)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it would reduce the time complexity, but it would work only when the agent ids are sequential, also it would create duplicate ids if the agents are removed, so the current implementation is more generalized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also it would create duplicate ids if the agents are removed
example
if you have agents with IDs 0,1,2,3,4 and then if suppose agent with id 2 is removed, you'd have 4 agents with IDs 0,1,3,4. using len(obj._agents ) here would give 4 as the next id however it has been already taken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! yup you are right!, I guess if we really had to ever reduce the TC we could always keep a maxval or something that keeps track of next_id (O(1) just something I thought of.
I believe it's better to generate unique IDs randomly instead of managing them manually. There's already another PR that's nearly complete and addresses this: #110 |
@adamamer20
Then its fine, I will close this PR. |
Actually, it's kind of the opposite unfortunately. In terms of space, Polars columns allocate a fixed size for integers—so if we use The main issue is more about management. We’re not only assigning IDs inside a single Also, every time we delete and add agents, sequential IDs would just keep increasing endlessly. On top of that, by using UUIDs with a fixed seed, we get reproducible IDs across runs, which is super useful when debugging or reproducing experiments. With sequential IDs, that would be harder to guarantee. So in the end, UUIDs actually make things easier and safer for us to maintain. |
oh yeah, then random generation makes sense. Thanks for the explanation! |
Make unique_ids optional by auto-generating them when not provided,
@adamamer20 I implemented the automatic unique_id generation for both Polars and pandas backends . While I understand pandas implementation will eventually be removed, should I keep it the same for now or wait for #125 to be merged.