-
Notifications
You must be signed in to change notification settings - Fork 1k
Stabilize experimental Cell Space as mesa.discrete_space
#2610
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
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
I think the deprecation warning needs to be updated, from the build failure in test examples it is saying "DeprecationWarning: you are importing from shouldn't this be "DeprecationWarning: you are importing from |
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.
@quaquel Thanks for doing this!
Before you merge could you update the tests or the pyproject so the build isn't failing with the deprecation warning.
I would like to take a good look at namespaces, also to make it futureproof for other network spaces etc. Hopefully I can do that Tuesday. Also we should split off a 3.1 maintenance branch before merging. |
Yes, this is very much on my list of things to do before merging.
No idea what you mean with this. There is a network in the current |
|
If possible, can you ensure the build passes, just because it may hide other issues, pytest should be able to handle it either in the test_examples with something like
or globally in the pyproject.toml
|
Currently the Otherwise, if you, Tom and other @projectmesa/maintainers agree, don't let me block you. |
you can actually use a shorter import: |
@tpike3: the failing action is the one that runs mesa-examples. So, this build runs all examples in the mesa-examples repo. All tests on the mesa repo itself, including the examples that ship with mesa itself, pass without problems. Our most complete integration test is erroring and only because of a deprecation warning. About the only thing I see that can be done to get the built to pass is to explicitly ignore the deprecation warnings by changing this line in built_lint.yml. Let me know what you think about that. |
Works for me, thanks! |
Ok, all tests now pass correctly so this is ready to be merged. However, I want to hold of for a short while still. (1). we need to first create a 3.1 maintenance branch, and (2) I ran into a small bug in hexgrid. I know how to resolve the bug, which is due to mixing up (row, column) indexing in hexgrid and (x, y) coordinates when visualizing. |
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.
Thanks for all your work so far. Perfect is the enemy of done, once it's in we can start updating the example models and tutorial. Let's drive this home!
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mesa.discrete_space
As part of the stabilization of cell spaces, this PR moves everything to
mesa.discrete_space
. I have kept themesa.experimental.cell_space
, which should continue to function but will issue a deprecation warning.I updated all tests and all examples that use cell spaces.