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

Adding multiMapping plots #237

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 57 additions & 1 deletion gufe/visualization/mapping_visualization.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
# This code is part of OpenFE and is licensed under the MIT license.
# For details, see https://github.com/OpenFreeEnergy/gufe
from typing import Any, Collection, Optional
from typing import Any, Collection, Optional, List
from itertools import chain
from PIL import Image, ImageOps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe pillow now replaces PIL, which should be used instead.

Additionally - we would need to work out what we want to do with this additional dependency. Probably would need to be an optional.

Copy link
Contributor

@jthorton jthorton Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use pillow I think rdkit has that as part of its recipe.

from io import BytesIO

from rdkit import Chem
from rdkit.Chem import Draw
Expand Down Expand Up @@ -305,3 +307,57 @@
bond_colors=[{}],
highlight_color=red,
)


def _concat_images(imgs, buffer_spacing=4, ncols=3)-> Image:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the function that was used to create the grid in the example? This should probably be used by the draw_multipule_mappings function to create a grid layout.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this PR is a very old one and I wanted to bring it to your attention as I think it could be a handy feature.

hm... I wonder if this function should be used in line 360 somehow? I also wonder if the code developed away from the initial implementation?

"""
This helper function is constructing a Grid Image of given imgs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if it's private, this really could use with a better explanation of what the method does.

"""
height = 0
width = 0

Check warning on line 317 in gufe/visualization/mapping_visualization.py

View check run for this annotation

Codecov / codecov/patch

gufe/visualization/mapping_visualization.py#L316-L317

Added lines #L316 - L317 were not covered by tests

#Calculate number of rows
nrows= (len(imgs)+1)//ncols if(len(imgs)%3>0) else len(imgs)//ncols

Check warning on line 320 in gufe/visualization/mapping_visualization.py

View check run for this annotation

Codecov / codecov/patch

gufe/visualization/mapping_visualization.py#L320

Added line #L320 was not covered by tests

#Get total widht and height of img
for img in imgs:
height = max(height, img.height)
width = max(width, img.width)

Check warning on line 325 in gufe/visualization/mapping_visualization.py

View check run for this annotation

Codecov / codecov/patch

gufe/visualization/mapping_visualization.py#L323-L325

Added lines #L323 - L325 were not covered by tests

height = height * nrows
width = width * ncols

Check warning on line 328 in gufe/visualization/mapping_visualization.py

View check run for this annotation

Codecov / codecov/patch

gufe/visualization/mapping_visualization.py#L327-L328

Added lines #L327 - L328 were not covered by tests

width += buffer_spacing*ncols
height += buffer_spacing*nrows

Check warning on line 331 in gufe/visualization/mapping_visualization.py

View check run for this annotation

Codecov / codecov/patch

gufe/visualization/mapping_visualization.py#L330-L331

Added lines #L330 - L331 were not covered by tests

#Construct Concatentated Image
res = Image.new("RGBA",(width,height))

Check warning on line 334 in gufe/visualization/mapping_visualization.py

View check run for this annotation

Codecov / codecov/patch

gufe/visualization/mapping_visualization.py#L334

Added line #L334 was not covered by tests

x = 0
y = 0
for i, img in enumerate(imgs):
img = ImageOps.expand(img, border=3, fill="grey")
res.paste(img,(x,y))
x += img.width + buffer_spacing

Check warning on line 341 in gufe/visualization/mapping_visualization.py

View check run for this annotation

Codecov / codecov/patch

gufe/visualization/mapping_visualization.py#L336-L341

Added lines #L336 - L341 were not covered by tests

if(i%ncols == 0 and i!=0):
y+= img.height + buffer_spacing
x=0

Check warning on line 345 in gufe/visualization/mapping_visualization.py

View check run for this annotation

Codecov / codecov/patch

gufe/visualization/mapping_visualization.py#L343-L345

Added lines #L343 - L345 were not covered by tests

return res

Check warning on line 347 in gufe/visualization/mapping_visualization.py

View check run for this annotation

Codecov / codecov/patch

gufe/visualization/mapping_visualization.py#L347

Added line #L347 was not covered by tests

def draw_multiple_mappings(mappings:List, out_file_path:str=None)->Image:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the type hints, for mappings and out_file_path, maybe change the second to be output_file. We should expose the grid layout settings as well to recreate the example.

"""
This function generates an image, consisting of multiple mapping visualization.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a public facing method, it would need better docstring, especially on the "what does it do and what are my parameters".

"""
views = []
for m in mappings:
view = draw_mapping(m._compA_to_compB, m.componentA.to_rdkit(),

Check warning on line 355 in gufe/visualization/mapping_visualization.py

View check run for this annotation

Codecov / codecov/patch

gufe/visualization/mapping_visualization.py#L353-L355

Added lines #L353 - L355 were not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would really need tests of some kind, even if they are smoke tests (although I do note that matplotlib has a series of methods to test image equality that could be useful here).

m.componentB.to_rdkit(),)
bio = BytesIO(view)
view = Image.open(bio)
views.append(view)
res = show_images(views)

Check warning on line 360 in gufe/visualization/mapping_visualization.py

View check run for this annotation

Codecov / codecov/patch

gufe/visualization/mapping_visualization.py#L357-L360

Added lines #L357 - L360 were not covered by tests

if(out_file_path is not None):
res.save(out_name)

Check warning on line 363 in gufe/visualization/mapping_visualization.py

View check run for this annotation

Codecov / codecov/patch

gufe/visualization/mapping_visualization.py#L362-L363

Added lines #L362 - L363 were not covered by tests
Loading