Skip to content
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

Feature Improve Plotting #107

Merged
merged 5 commits into from Jun 20, 2023
Merged

Conversation

fjclark
Copy link
Contributor

@fjclark fjclark commented Jun 16, 2023

Is this pull request to fix a bug, or to introduce new functionality?

This PR attempts to improve the Notebook plotOverlap function:

a) To be more consistent with Fig 13 in the best practices guide. The main change is to use the same overlap cut-offs as in the best practices paper, which were not being used before. This led to misleading plots like this (despite reasonable overlap for all first off-diagonal windows) (text removed and colour bar added):

image

Where points with reasonable overlap would be grouped together with points with very low overlap. A colour bar is also added.

b) To avoid covering the whole plot in text when many windows are supplied. I've checked with 4, 16, and 36 lambda windows and the output looks reasonable in each case:

image
image
image

c) To increase flexibility so that a numpy array (as is returned from the analysis functions) can be passed directly.

This addresses comments here.

  • I confirm that I have merged the latest version of devel into this branch before issuing this pull request (e.g. by running git pull origin devel):y
  • I confirm that I have added a test for any new functionality in this pull request: n
  • I confirm that I have added documentation (e.g. a new tutorial page or detailed guide) for any new functionality in this pull request: n
  • I confirm that I have permission to release this code under the GPL3 license: y

Suggested reviewers:

@lohedges

Makes plot more consistent with best practices and ensures that
using many lambda windows doesn't obscure the whole plot with text.
@fjclark
Copy link
Contributor Author

fjclark commented Jun 16, 2023

Sorry, forgot to skip the CI on this and with minor tweaks to by SOMD ABFE PR - will remember in future.

@lohedges
Copy link
Contributor

Thanks for this. I'll hop on your branch and have a play around locally. No worries, I just skipped the CI since this isn't covered by any test.

@lohedges
Copy link
Contributor

Is it possible to reduce the height of the colour bar so that it matches that of the heatmap? This would avoid a bunch of whitespace to the top and bottom, which would make it much easier to include in a publication, or when making a montage with other plots.

@lohedges
Copy link
Contributor

I think it would look nicer if the x-axis label ran along the top, which would make the (0, 0) point the top-left corner. This appears to be what is done in the best practices paper. (The previous plot in BSS had the origin at the bottom left.)

@lohedges
Copy link
Contributor

It also looks like the best practices plot removes the border and sets the grid colour to white. I wonder how that would look? I guess you would still end up with the same issue if there were many windows, i.e. the white grid making things hard to see.

@fjclark
Copy link
Contributor Author

fjclark commented Jun 16, 2023

Thanks for the comments. How do these look?

image
image

@lohedges
Copy link
Contributor

Nice. I think those look much cleaner.

One other thought. Do you think it could be useful to make the magic 0.3 "good overlap" cut-off a user configurable parameter with a default of 0.3? I wonder if there are situations where it would be desirable to run a quick simulation knowing there is a lower level of accuracy, or if people have different opinions as to what is an acceptable value? This is also related to @annamherz's analysis work where there is a function to check the overlap is sufficient. In that case it might also be desirable to have a user configurable option with a default of 0.3. (Having an exposed option would also make it much easier to tweak things if the "best practice" evolves over time )

@fjclark
Copy link
Contributor Author

fjclark commented Jun 16, 2023

Have also hidden the colour bar outline for consistency, e.g.

image

@fjclark
Copy link
Contributor Author

fjclark commented Jun 16, 2023

Great.

One other thought. Do you think it could be useful to make the magic 0.3 "good overlap" cut-off a user configurable parameter with a default of 0.3? I wonder if there are situations where it would be desirable to run a quick simulation knowing there is a lower level of accuracy, or if people have different opinions as to what is an acceptable value? This is also related to @annamherz's analysis work where there is a function to check the overlap is sufficient. In that case it might also be desirable to have a user configurable option with a default of 0.3. (Having an exposed option would also make it much easier to tweak things if the "best practice" evolves over time )

Do you mean 0.03? (https://link.springer.com/article/10.1007/s10822-015-9840-9)

Personally I think I might prefer a continuous rather than a discrete colour map as blocking the 0.1 - 0.3 windows together seems quite large, although maybe the discrete colours make things clearer. Maybe we could allow the user to select discrete or continuous colour bars, and supply a list of boundaries if discrete? Or is this over-complicating things?

@lohedges
Copy link
Contributor

Yes 0.03 🤦‍♂️

Personally I think I might prefer a continuous rather than a discrete colour map as blocking the 0.1 - 0.3 windows together seems quite large, although maybe the discrete colours make things clearer. Maybe we could allow the user to select discrete or continuous colour bars, and supply a list of boundaries if discrete? Or is this over-complicating things?

Yeah, not sure. As you say, perhaps it overcomplicates things, but it's probably easy enough to support. I'll have a think.

@fjclark fjclark temporarily deployed to biosimspace-build June 16, 2023 20:52 — with GitHub Actions Inactive
@fjclark fjclark temporarily deployed to biosimspace-build June 16, 2023 20:52 — with GitHub Actions Inactive
@fjclark fjclark temporarily deployed to biosimspace-build June 16, 2023 20:52 — with GitHub Actions Inactive
@fjclark fjclark temporarily deployed to biosimspace-build June 16, 2023 20:52 — with GitHub Actions Inactive
@fjclark fjclark temporarily deployed to biosimspace-build June 16, 2023 20:52 — with GitHub Actions Inactive
@fjclark
Copy link
Contributor Author

fjclark commented Jun 16, 2023

Ok, I've added an options for continuous or discrete color mappings, and user-specified cut-off values. The discrete default looks like:

image

While the continuous equivalent is:

image

I haven't managed to get the correspondence between the continuous and discrete bars great yet. I'm away next week so feel free to merge and modify if you have the time.

Cheers.

@lohedges
Copy link
Contributor

Many thanks. I'll merge as is since it meets the spec. We can think about tweaks at a later date.

@lohedges lohedges merged commit 099ca0d into OpenBioSim:devel Jun 20, 2023
5 checks passed
@lohedges lohedges mentioned this pull request Jun 21, 2023
@annamherz
Copy link
Contributor

This is also related to @annamherz's analysis work where there is a function to check the overlap is sufficient. In that case it might also be desirable to have a user configurable option with a default of 0.3. (Having an exposed option would also make it much easier to tweak things if the "best practice" evolves over time )

I think that's a good idea! I've added it for now in the branch.

Currently in the analysis branch, the plotting uses the plotting functions from alchemlyb for the overlap and dhdl, but I think this plotting for the overlap looks better with the colours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants