-
Notifications
You must be signed in to change notification settings - Fork 5
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
Adds live plot context manager and update events #49
Conversation
ac8f4ea
to
94483fb
Compare
94483fb
to
9b1a3eb
Compare
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 can confirm that the plots behave as expected and discussed both when importing and running the examples as a script. I have tested this using both pycharm and the windows x64 command prompt.
Alongside the comments could you please review the test coverage for "plotting.py", in particular could you please write a test for "plot_ref_sld".
RATapi/utils/plotting.py
Outdated
@@ -75,33 +37,38 @@ def plot_errorbars(ax: Axes, x: np.ndarray, y: np.ndarray, err: np.ndarray, one_ | |||
ax.scatter(x=x, y=y, s=3, marker="o", color=color) | |||
|
|||
|
|||
def plot_ref_sld_helper(data: PlotEventData, fig: Optional[Figure] = None, delay: bool = True): | |||
def plot_ref_sld_helper(data: PlotEventData, fig: Optional[matplotlib.pyplot.figure] = None, delay: bool = True): |
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.
Whilst we're here . . .
Could we review the axis labels - I used:
ref_plot.set_xlabel("q_{z} (\u00c5^{-1})")
ref_plot.set_ylabel("R(q_{z})") - we could also go for "Reflectivity"
sld_plot.set_xlabel("z (\u00c5)")
sld_plot.set_ylabel("SLD (\u00c5^{-2})")
for my poster. I think they'd be better than what we have. Similarly, for the legend labels, surely it'd be better to use the name of each contrast, rather than "ref 1, ref 2" etc - we could pass them in through the PlotEventData
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.
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.
Ahh, so close. The ^ and _ are meant to be superscripts and subscripts. good point about units we should check with @arwelHughes, but I'm not aware of how one would change the units in the code.
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 updating the labels via PlotEventData will require me diving into the RAT, C++ to add more stuff so I will do that in another PR but I will update the labels and add a unit test for plot_ref_sld
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.
Ahh, so close. The ^ and _ are meant to be superscripts and subscripts.
This works
ref_plot.set_xlabel("$q_{z} (\u00c5^{-1})$")
ref_plot.set_ylabel("$Ref (q_{z})$")
sld_plot.set_xlabel("$Z (\u00c5)$")
sld_plot.set_ylabel("$SLD (\u00c5^{-2})$")
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.
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.
Ahh, no I had it that the reflectivity is a function of qz, but obviously it's a bit confusing like this. Maybe best to leave that off then.
tests/test_plotting.py
Outdated
@patch("RATapi.utils.plotting.plot_ref_sld_helper") | ||
def test_plot_ref_sld(mock: MagicMock, request) -> None: | ||
problem = request.getfixturevalue("input_project") | ||
results = request.getfixturevalue("reflectivity_calculation_results") |
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 strongly recommended way to request fixtures is to include the them in the function arguments. This method of requesting fixtures is for when the choice of fixture depends on other parameters. I've seen that line 554 of "test_run.py" has a line like this as well (no idea who wrote it . . .) - could you please also change that to include the fixture in the function argument.
a7021f4
to
a3f5192
Compare
Adds a live plot as a context manager, to use change the controls
then run like so
or