-
Notifications
You must be signed in to change notification settings - Fork 29
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 dashed red diagonal on scatterplot #63
Conversation
a4e23e5
to
c071152
Compare
c4e60fc
to
24595af
Compare
def add_identity(self, axes, *line_args, **line_kwargs): | ||
(identity,) = axes.plot([], [], *line_args, **line_kwargs) | ||
|
||
def callback(axes): |
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.
Not sure I understand why the callback is needed
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.
The call back function keeps track of potential future changes in the axis format, and calls back this function to re-run and ensure that the red line is reformatting according to the new display.
For instance, if you later change the range of the axis, you want to make sure that the red line extends or contract based on the new axis limit. Instead of doing that manually, the callbacks performs the re-adjustment automatically.
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.
Makes sense. I tried it out - very fancy. I'll open an issue to make sure you can actually interact with plots rather than only save them
@@ -550,6 +553,21 @@ def create_scatter_plot_nodes_vec( | |||
fig.savefig(f"./logs/{self.model_with_config_name}/" + varname + ".png") | |||
plt.close() | |||
|
|||
def add_identity(self, axes, *line_args, **line_kwargs): |
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.
Is it bad to just plot a line y=x
? I'm not familiar with the approach here
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.
This does indeed y=x. But you need to make sure that the y=x is extended through the entire range of the plot frame.
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.
Looks good to me. Only one comment about moving the identity plot inside if
@@ -124,6 +125,8 @@ def __scatter_impl( | |||
ax.set_xlim(minimum, maximum) | |||
ax.set_ylim(minimum, maximum) | |||
|
|||
self.add_identity(ax, color="r", ls="--") |
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.
Move this inside if xylim_equal:
?
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.
No, I think we want this regardless of forcing x_lim = y_lim
* added dashed red diagonal and use of empty dots in scatterplot * formatting fixed Co-authored-by: Massimiliano Lupo Pasini <7ml@ornl.gov>
Also makes scatter plot empty markers