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

Integrate lasso highlight viz functions #177

Merged
merged 16 commits into from
Apr 13, 2023
Merged

Conversation

dessygil
Copy link
Contributor

@dessygil dessygil commented Apr 5, 2023

I am still working on this. I want to add test cases and improve the code. I have the Lasso highlight function working only for the ideal scenarios, but I wanted to ensure I was on the right track regarding what's expected. I've extended this tool to abstract the work required from the original program; now, it can be called by a single function. The tests that I thought would be worthwhile that sit outside testing the actual image can be found in Visualization.ipynb. I'm curious which tests would actually be worth adding (still a little new to open source and testing). After this, I plan to get items 2-4 done on the checklist.

Checklist:

  • Add tests to cover the fixed bug(s) or the new introduced feature(s) (if appropriate).
  • Update the API documentation is a new function is added or an existing one is deleted.
  • Added a news entry.
    • copy news/TEMPLATE.rst to news/my-feature-or-branch.rst) and edit it.

@dessygil dessygil requested a review from hadim as a code owner April 5, 2023 22:05
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Merging #177 (25d216b) into main (a923213) will decrease coverage by 0.07%.
The diff coverage is 89.96%.

@@            Coverage Diff             @@
##             main     #177      +/-   ##
==========================================
- Coverage   91.08%   91.01%   -0.07%     
==========================================
  Files          47       48       +1     
  Lines        3509     3785     +276     
==========================================
+ Hits         3196     3445     +249     
- Misses        313      340      +27     
Flag Coverage Δ
unittests 91.01% <89.96%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
datamol/data/__init__.py 79.51% <80.76%> (ø)
datamol/viz/_lasso_highlight.py 90.63% <90.63%> (ø)
datamol/__init__.py 100.00% <100.00%> (ø)
datamol/fragment/_assemble.py 72.40% <100.00%> (ø)
datamol/viz/__init__.py 100.00% <100.00%> (ø)
datamol/viz/_circle_grid.py 78.70% <100.00%> (+0.40%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@MichelML
Copy link
Contributor

MichelML commented Apr 6, 2023

@dessygil thanks for this contribution, really enjoyed your notebook, small question after going quickly through the code: does this support SVG depictions, and if not, how complex would it be to support it?

@dessygil
Copy link
Contributor Author

dessygil commented Apr 6, 2023

Hi @MichelML !

It does not right now, I have it working in a Jupyter Notebook so far but I'm guessing it would be similar to this piece of code below from to_image in datamol/viz/_viz.py?

if outfile is not None:
        with fsspec.open(outfile, "wb") as f:
            if use_svg:
                if isinstance(image, str):
                    # in a terminal process
                    f.write(image.encode())  # type: ignore
                else:
                    # in a jupyter kernel process
                    f.write(image.data.encode())  # type: ignore
            else:
                if isinstance(image, PIL.PngImagePlugin.PngImageFile):  # type: ignore
                    # in a terminal process
                    image.save(f)
                else:
                    # in a jupyter kernel process
                    f.write(image.data)  # type: ignore

    return image

@dessygil
Copy link
Contributor Author

dessygil commented Apr 7, 2023

@MichelML I was wrong in assuming it would be easy, but I have an idea. The final function I utilize to return an image is

GetDrawingText((MolDraw2DCairo)arg1) → object :[¶](https://www.rdkit.org/docs/source/rdkit.Chem.Draw.rdMolDraw2D.html#rdkit.Chem.Draw.rdMolDraw2D.MolDraw2DCairo.GetDrawingText)
return the PNG data as a string

The RDKit docs state that the PNG data is returned as a string and using io.BytesIO returns the PNG binary. I could use a library called Wand (or Potrace and PyCairo).

The way I would use wand is shown below.

from wand.image import Image

with Image(blob=png_binary) as img:
    img.format = 'svg'
    img.save(filename='output.svg')

Whether I save it to a file or return it this could be a possible way of dealing with providing an SVG depiction.

Please let me know if I can download another dependency or if there is a better way to approach this?

@cwognum
Copy link
Contributor

cwognum commented Apr 7, 2023

Hi @dessygil,

Thank you for your contributions so far!

Just commenting on the how we could support SVG. I think this is doable by (conditionally, e.g. based on a use_svg parameter in the method) changing the MolDraw2DCairo to a MolDraw2DSVG object (see the RDKit docs). Since both base the same superclass, I expect the interface to be compatible and for this to be a relatively easy change. Does that make sense?

datamol/viz/_lasso_highlight.py Outdated Show resolved Hide resolved
datamol/viz/_lasso_highlight.py Outdated Show resolved Hide resolved
datamol/viz/_lasso_highlight.py Outdated Show resolved Hide resolved
datamol/viz/_lasso_highlight.py Outdated Show resolved Hide resolved
datamol/viz/_lasso_highlight.py Outdated Show resolved Hide resolved
datamol/viz/_lasso_highlight.py Outdated Show resolved Hide resolved
datamol/viz/_lasso_highlight.py Outdated Show resolved Hide resolved
datamol/viz/_lasso_highlight.py Outdated Show resolved Hide resolved
datamol/viz/_lasso_highlight.py Outdated Show resolved Hide resolved
datamol/viz/_lasso_highlight.py Outdated Show resolved Hide resolved
@hadim
Copy link
Contributor

hadim commented Apr 7, 2023

@dessygil : there are some changes to be made, but it's going in the right direction. Please let us know if you have any questions.

@hadim hadim linked an issue Apr 13, 2023 that may be closed by this pull request
@hadim hadim changed the title First commit for lassohighlight Integrate lasso highlight viz functions Apr 13, 2023
@hadim hadim self-requested a review April 13, 2023 17:57
Copy link
Contributor

@hadim hadim left a comment

Choose a reason for hiding this comment

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

Thank you so much for the work and your patience here @dessygil!

I believe this will be quite a useful feature for our datamol users!

The reason I am being picky on the typing is because 1) it makes code maintenance much easier and 2) also facilitates consuming the datamol API for our downstream users.

(I also took the freedom to push a few commits that are not really related to that PR xD)

I will merge here once green. Thanks again!

@dessygil
Copy link
Contributor Author

Thank you so much for the work and your patience here @dessygil!

I believe this will be quite a useful feature for our datamol users!

The reason I am being picky on the typing is because 1) it makes code maintenance much easier and 2) also facilitates consuming the datamol API for our downstream users.

(I also took the freedom to push a few commits that are not really related to that PR xD)

I will merge here once green. Thanks again!

Of course, I appreciate it. It helps me see what I need to do a bit better! Thanks to you and the team for jumping in whenever I needed help. I look forward to giving the next issue a go :)

@hadim hadim merged commit 41a0b58 into datamol-io:main Apr 13, 2023
16 checks passed
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.

Integrate lassohighlight
5 participants