Solvation plots #67
Solvation plots #67
Conversation
# Conflicts: # requirements.txt # solvation_analysis/solution.py # solvation_analysis/tests/test_analysis_library.py
# Conflicts: # solvation_analysis/tests/test_analysis_library.py
…solvation-analysis into visualization_module
# Conflicts: # solvation_analysis/tests/conftest.py
# Conflicts: # solvation_analysis/tests/conftest.py
|
Hello @laurlee! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-08-17 19:38:59 UTC |
solvation_analysis/plotting.py
Outdated
| # histogram of what? | ||
| return | ||
|
|
||
| def format_graph(fig, title, x_axis, y_axis): |
There was a problem hiding this comment.
This feels a little bit unnecessary. I would just update these in the graph when required. :)
There was a problem hiding this comment.
What do you think of the idea of a general templating function? I am thinking each function could return a fairly bare bones plot that could then be prettied up with some formatting function.
def format_plot(fig)
# format fig
return fig
fig = plot_something(data)
pretty_fig = format_plot(fig)This might be overly complicated, but we could also make it a decorator.
def pretty_plot(plotting_func):
def pretty_plotting_func(data):
fig = plotting_func(data)
# format fig
return fig
return pretty_plotting_funcwe could then add the formatting decorator to our definition of each plot. This would allow solvation_analysis to standardize a particular plotting style (I have become quite opinionated on the subject) with maximum code reuse.
@pretty_plot
def plotting_func(data)
# make fig
return fig To be clear, this is a bit different from what's currently in the code.
There was a problem hiding this comment.
Sounds like a good idea to me!
solvation_analysis/plotting.py
Outdated
| def plot_speciation(solution): | ||
| # square area | ||
| # should be doable with plotly.express.imshow and go.add_annotations | ||
| return |
There was a problem hiding this comment.
Nice progress @laulee! A few comments, some of which we have already discussed.
|
|
||
| def test_plot_network_size_histogram(networking): | ||
| fig = plot_network_size_histogram(networking) | ||
| fig.show() |
There was a problem hiding this comment.
I don't think you ever need to have show() in library code (and certainly not in the tests): either you can return the fig object and then the user can do what they want (including show) or your code plot to files and generates files (and then it slows you down a lot to run show).
| def test_plot_network_size_histogram(networking): | ||
| fig = plot_network_size_histogram(networking) | ||
| fig.show() | ||
| assert True |
There was a problem hiding this comment.
I'd always assert something meaningful, e.g., that this is the correct instance, or you can dig into the figure object and check that it has the right labels, or that the data is what you expect it to be. But do a tiny bit more than flushing code paths.
…ed compare_pairing and compare_coordination_numbers with new **kwargs setup
solvation_analysis/plotting.py
Outdated
| fig : Plotly.Figure | ||
|
|
||
| """ | ||
| fig.update_layout(xaxis_title_text=x_axis.title(), yaxis_title_text=y_axis.title(), title=title.title()) |
There was a problem hiding this comment.
I'd favor removing the format_graph function and just moving this to plot_pairing, plot_coordination etc. That will reduce the number of kwargs to compare_solvent_dicts and reduce the number of functions.
solvation_analysis/plotting.py
Outdated
| """ | ||
| # catch_different_solvents(solutions) | ||
| residence = [solution.residence.residence_times for solution in solutions] | ||
| residence = set(residence) - set(ignore) |
There was a problem hiding this comment.
is this an old implementation?
There was a problem hiding this comment.
Yes, I'll update it to reflect the current implementation when I next push the code.
…rmatting, added implementation to coordination and residence, and added more tests
There was a problem hiding this comment.
Nice work @laurlee! I have a few comments mostly related to variable naming and readability. I think the implementation all looks good.
The main remaining challenge before merging this will be updating to v0.1.4 which has a number of API changes.
solvation_analysis/plotting.py
Outdated
| if keep_solvents: | ||
| for solution in properties: | ||
| try: | ||
| properties[solution] = {keep: properties[solution][keep] for keep in keep_solvents} | ||
| except KeyError: | ||
| # if argument for keep_solvents is invalid | ||
| raise Exception("Invalid value of keep_solvents. \n keep_solvents: " + | ||
| str(keep_solvents) + "\n Valid options for keep_solvents: " + | ||
| str(list(properties[solution].keys()))) from None |
There was a problem hiding this comment.
If possible, let's try to avoid the try except loop.
Look into Dict.get() which returns the item if it exists and returns None if not. It's a very convenient function!
solvation_analysis/plotting.py
Outdated
| if res_type == "residence_times": | ||
| res_time = {solution: solutions[solution].residence.residence_times for solution in solutions} | ||
| elif res_type == "residence_times_fit": | ||
| res_time = {solution: solutions[solution].residence.residence_times_fit for solution in solutions} | ||
| else: | ||
| raise ValueError("res_type must be either \"residence_times\" or \"residence_times_fit\"") |
There was a problem hiding this comment.
lets use Solution.hasattr("residence") to check if residence is present.
# Conflicts: # solvation_analysis/analysis_library.py # solvation_analysis/tests/test_analysis_library.py
Description
Provide a brief description of the PR's purpose here.
Todos
Notable points that this PR has either accomplished or will accomplish.
Questions
Status