-
Notifications
You must be signed in to change notification settings - Fork 925
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
rename make_plot_measure to make_plot_component and add some kwargs #2446
Conversation
Performance benchmarks:
|
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.
Can we let make_plot_measure
throw a warning that it is replaced with make_plot_component
? That way people using the betas have some clue how to fix their models.
The migration guide might also need an update.
We made this change for space already and did not bother with maintaining backward compatability. Given that all this Solara stuff is breaking old stuff left, right, and center anyway, I would prefer to just clean up rather than keep stuff floating around for the few people using pre-releases. I agree that as part of mesa 3.0 release, we need to add a section in the migration guide on this. |
@@ -618,30 +618,46 @@ def _scatter(ax: Axes, arguments): | |||
) | |||
|
|||
|
|||
def make_plot_measure(measure: str | dict[str, str] | list[str] | tuple[str]): |
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.
Letting this function call a warning / informative error is an oneliner right?
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.
and more lines to move around in some of the other refactoring still to come to address other open issues.
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.
I just checked, None of this is in 2.4 only in the mesa 3 alpha's and beta's. So I really don't see the point of bothering with warnings. I am going to move more stuff around for example for #2434. I agree that warnings etc. are in order for official releases and when deprecating stable stuff. This has never been officially stable yet anyway.
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.
I don’t like such breaking changes without proper error handling so late in the beta release cycle, but since we never released this as stable before I find it acceptable if it’s your strong preference.
Don’t expect me to make an habit out of it ;)
If I knew we were going to do such extensive overhauls I would have suggested staying longer in alpha, maybe something to more explicitly discuss if/when 4.0 is targeted.
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.
I did not know that either until I started looking at this stuff.
Do you have any sense of how many downloads there have been for some of the pre releases?
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, we were more or less in agreement that the components part of solara would be experimental in 3.0 anyway 😄
I agree with @EwoutH to keep the old function names as deprecated aliases for the new functions. Apart from the name change it should still just work, right? No need to cause more change than needed. Yes it was always experimental but we recommended it for quite some time now, since mesa 2.2 or something? Yes it's a bit more code to move around, but I think it's a good trade off. And then immediately delete it for 3.1 |
I added the deprecation warning (also for make_space_component / make_space_matplotlib ) |
Thanks! |
Well handled, appreciated! One minor note, I think the renames in the |
Good catch, we should handle this when we rename the modules |
exactly my plan |
This renames make_plot_measure to make_plot_component and adds 2 new keyword arguments.
post_process
is identical to the one used inmake_space_component
. The save_format closes another open issue. See virus_on_a_network for using post_process.closes #2443, #1823, #1537, #795