Plot equation properties #175
Conversation
pysr/sr.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| threshold : float |
There was a problem hiding this comment.
These are out of order. Also, fontsize is missing.
More generally, it would be good to add **kwargs to the plotting function to tweak the other settings.
pysr/sr.py
Outdated
| ax.set_xticklabels(complexities_plot); | ||
|
|
||
| # label on the axis: | ||
| ax.set_xlabel('Complexity ', fontsize=10); |
There was a problem hiding this comment.
This doesn't use the fontsize variable.
| Parameters | ||
| ---------- | ||
| threshold : float | ||
| The plotting threshold. Plots only equations that with a score |
There was a problem hiding this comment.
I think this metric is too restrictive. Would be better to simply plot all equations (default) or a user-passed list of indices.
There was a problem hiding this comment.
I think plotting all will be too much of a mess (in most cases). I could change it to "number of equations to plot?"
There was a problem hiding this comment.
Yeah that is a good idea! Then the default would be, say, 5 equations - the 5 with the highest score. If there are less than 5 equations, it would be equal to the number of equations. (i.e., the parameter would be default to None and the number inferred).
|
Thanks, added some comments. Also, after those - could you add a unit-test to make sure it works as expected? |
|
|
||
| if ax is None: | ||
| import matplotlib.pyplot as plt | ||
| ax = plt.gca() |
There was a problem hiding this comment.
I think plt.gca() just gets the current/last axis, rather than creating a new one. But I think we would want this to generate a new figure/axis.
|
Actually, I like |
MilesCranmer
changed the title
|
Actually, I wonder if it would be better to allow the user to specify what column they wish to plot? i.e., |
|
Closing this as the upcoming tensorboard support will make this much easier to set up. Likely need to rewrite with that in mind (I will take care of this). Thanks @Pablo-Lemos! |
Similar to #156, as you suggested @MilesCranmer