Conversation
RiesBen
changed the title
RiesBen
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #237 +/- ##
==========================================
- Coverage 98.73% 97.31% -1.42%
==========================================
Files 36 36
Lines 2049 2083 +34
==========================================
+ Hits 2023 2027 +4
- Misses 26 56 +30 ☔ View full report in Codecov by Sentry. |
| ) | ||
|
|
||
|
|
||
| def _concat_images(imgs, buffer_spacing=4, ncols=3)-> Image: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
|
||
| return res | ||
|
|
||
| def draw_multiple_mappings(mappings:List, out_file_path:str=None)->Image: |
There was a problem hiding this comment.
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.
jthorton
left a comment
jthorton
left a comment
There was a problem hiding this comment.
I really like this idea, it offers a quick way to see all the edges and mappings in the network without iterating through the network or using the CLI viewer! It just needs a bit more work to enable visuals like the great example. Do you have time to finish this @RiesBen or is this something that should be handed over to me and Hannah?
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Various comments on dependencies, tests, and documentation.
|
|
||
| def draw_multiple_mappings(mappings:List, out_file_path:str=None)->Image: | ||
| """ | ||
| This function generates an image, consisting of multiple mapping visualization. |
There was a problem hiding this comment.
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(), |
There was a problem hiding this comment.
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).
|
|
||
| def _concat_images(imgs, buffer_spacing=4, ncols=3)-> Image: | ||
| """ | ||
| This helper function is constructing a Grid Image of given imgs. |
There was a problem hiding this comment.
Even if it's private, this really could use with a better explanation of what the method does.
| from typing import Any, Collection, Optional | ||
| from typing import Any, Collection, Optional, List | ||
| from itertools import chain | ||
| from PIL import Image, ImageOps |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If we use pillow I think rdkit has that as part of its recipe.
Hi @jthorton , I sadly will not have time to continue here :( |
3 participants
This pull request adds obne nice small feature to gufe mapping visualization, such you can show multiple mappings on one figure:
draw_multiple_mappings- drafty right now.