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

compare_view widget and colab support #41

Merged
merged 30 commits into from
Dec 9, 2022
Merged

Conversation

amorgun
Copy link
Contributor

@amorgun amorgun commented Nov 19, 2022

I tried using this library in colab and encountered several issues:

  1. Google colab uses python3.7 and older versions of PIL and Jinja2, so I downgraded the required versions. You cannot simply upgrade the versions, because PIL is imported by default and without notebook restart import jupyter_compare_view fails on version check.
  2. Google colab uses different html classes and get_current_notebook script does not work there. Changing selectors didn't help so I removed them altogether and embedded a copy of compare_view in every result.
    It's the same issue as in the Images and image widget not appearing. #40, so this PR should also fix it.

I also added compare function to create a widget without the cell magic.

@kolibril13
Copy link
Member

kolibril13 commented Nov 19, 2022

Hi @amorgun,
thanks a lot for the contribution!
I've set the requirements arbitrary, so downgraded the required versions looks good to me!
Regarding the custom widget: Super cool! That is one issue that gets solved with your pr #16
Regarding the further procedure: I'd like to test the widget properly, but that I can do only in about 2 weeks, I am quite busy at the moment.
But the requirements can already be merged.
Therefore, the question to you:
Could you split this pr into two PRs?
One with requirements+renaming. The other one with the widget.
The first pr I can review soon, and then the second pr I will review somewhere in the near future.
Another advantage of this procedure:
Because renaming of the file + adding code at the same time makes it difficult for me to see what code you wrote new, and what is already existing code.

@kolibril13
Copy link
Member

I will have time for testing and reviewing this pr next weekend.
@christopher-besch, @ianhi, I'd like to ask you: Do you have time for a review? :)
I just gave the widget a try, and it even works in VS Code (corresponding issue: #6), that is amazing!
This pr is also linked to widget issue #16
@amorgun : Can you update the pr title, e.g. "compare_view widget and colab support" and maybe also update the pr description?

@amorgun amorgun changed the title Make compare_view compatible with colab compare_view widget and colab support Nov 27, 2022
Copy link
Member

@kolibril13 kolibril13 left a comment

Choose a reason for hiding this comment

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

I've now had some time to test this your implementation and gave some comments.
Thanks for your great work, I think this can be merged soon! :)
Another thing:
Maybe we can add this into the example notebook:

from jupyter_compare_view import compare
from skimage import data
from skimage.color import rgb2gray
import matplotlib.pyplot as plt
img = data.chelsea()
grayscale_img = rgb2gray(img)
compare([img,grayscale_img], add_controls=False)

ImageLike = typing.TypeVar('ImageLike')


def img2bytes(img: ImageLike, format: str = 'jpeg') -> bytes:
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to choose 'jpeg' over 'png' here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason. But now I added it as a compare argument so it can be altered.

# anything other that can be displayed with plt.imshow
import matplotlib.pyplot as plt

plt.imsave(im_file, img, format=format)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
plt.imsave(im_file, img, format=format)
plt.imsave(im_file, img, format=format, cmap= "gray")

otherwise, gray images would look like this
image

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 think it's better to respect rcParams['image.cmap'] so I added cmap argument to compare.

jupyter_compare_view/compare.py Outdated Show resolved Hide resolved
jupyter_compare_view/compare.py Show resolved Hide resolved
jupyter_compare_view/__init__.py Outdated Show resolved Hide resolved
* Update __init__.py

* Update compare.py

* Update sw_cellmagic.py

* Update compare.py

* Update compare.py

* add widget exaple

* Update compare.py

* Add cmap arg
@kolibril13
Copy link
Member

The widget is really great now! Thanks for these quick changes.
One thing I've noted: The %%compare cell magic is no longer working:
image
There is no image showing, and multiple options can be selected at the same time, which should not be the case.
Furthermore, when running the cellmagic with an argument, I currently get this:

%%compare  --config '{"circle_size": 30}' 

plt.imshow(img)
plt.axis("off")
plt.show()

plt.imshow(grayscale_img, cmap="gray")
plt.axis("off")
plt.show()
---------------------------------------------------------------------------
JSONDecodeError                           Traceback (most recent call last)
Input In [3], in <cell line: 1>()
----> 1 get_ipython().run_cell_magic('compare', ' --config \'{"circle_size": 30}\'', '\nplt.imshow(img)\nplt.axis("off")\nplt.show()\n\nplt.imshow(grayscale_img, cmap="gray")\nplt.axis("off")\nplt.show()\n')

File ~/projects/jupyter_compare_view/compare_view_env/lib/python3.9/site-packages/IPython/core/interactiveshell.py:2358, in InteractiveShell.run_cell_magic(self, magic_name, line, cell)
   2356 with self.builtin_trap:
   2357     args = (magic_arg_s, cell)
-> 2358     result = fn(*args, **kwargs)
   2359 return result

File ~/projects/jupyter_compare_view/jupyter_compare_view/sw_cellmagic.py:64, in CompareViewMagic.compare(self, line, cell)
     58 args = magic_arguments.parse_argstring(CompareViewMagic.compare, line)
     59 height = args.height
     61 return compare(
     62     *out_images_base64,
     63     **{
---> 64         **json.loads(args.config),
     65         "height": height if height == "auto" else int(height)
     66     }
     67 )

File /opt/homebrew/Cellar/python@3.9/3.9.15/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/__init__.py:346, in loads(s, cls, object_hook, parse_float, parse_int, parse_constant, object_pairs_hook, **kw)
    341     s = s.decode(detect_encoding(s), 'surrogatepass')
    343 if (cls is None and object_hook is None and
    344         parse_int is None and parse_float is None and
    345         parse_constant is None and object_pairs_hook is None and not kw):
--> 346     return _default_decoder.decode(s)
    347 if cls is None:
    348     cls = JSONDecoder

File /opt/homebrew/Cellar/python@3.9/3.9.15/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/decoder.py:337, in JSONDecoder.decode(self, s, _w)
    332 def decode(self, s, _w=WHITESPACE.match):
    333     """Return the Python representation of ``s`` (a ``str`` instance
    334     containing a JSON document).
    335 
    336     """
--> 337     obj, end = self.raw_decode(s, idx=_w(s, 0).end())
    338     end = _w(s, end).end()
    339     if end != len(s):

File /opt/homebrew/Cellar/python@3.9/3.9.15/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/decoder.py:355, in JSONDecoder.raw_decode(self, s, idx)
    353     obj, end = self.scan_once(s, idx)
    354 except StopIteration as err:
--> 355     raise JSONDecodeError("Expecting value", s, err.value) from None
    356 return obj, end

JSONDecodeError: Expecting value: line 1 column 1 (char 0)

@kolibril13
Copy link
Member

Thanks for these changes.
The error message for cells with arguments is now gone, however the images still don't show in JupyterLab.
Screenshot from this branch:
image
Neither in Jupyter for VSCode (in VS Code they actually never worked before, #6):
Screenshot from this branch:
image

* Update sw_cellmagic.py

* Update sw_cellmagic.py

* Update compare.py
@amorgun
Copy link
Contributor Author

amorgun commented Dec 5, 2022

I think cell magic is fixed now. I tested it in colab and JupyterLab, but I don't have VSCode configured to test it.

Copy link

@ianhi ianhi left a comment

Choose a reason for hiding this comment

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

Hey!

This looks great and provides basically all of the functionality I was after in #16 and I definitely recommend merging and releasing :)

That said, I think it's important to note that this is not actually making an ipywidget. The simple way to see this is that the return object does not inherit from ipywidgets.Widget. The practical consequence of this is two fold:

First, It may be difficult to embed the compare view into a larger widget layout.

Second, there is no way to interact programmatically with the display, and you cannot re-use the same object. The advantage of going to make a widget would be that the following code would all be possible with the compare_view updating in sync with the code:

import ipywidgets as widgets
from jupyter_compare_view import CompareView

# create object
comparer = CompareView(img1, img2)


# comparer is a subclass of Widget which lets you do things
# like embed it in a widget layout, and interact with parameters programmatically
assert isinstance(widgets.Widget, comparer)

display(widgets.HBox([widgets.Button(...), comparer]))



# query the current slider position:
print(comparer.slider_position)

# set slider position
comparer.slider_position = .75

# change control type
comparer.mode = "circle"


# swap in a different image
comparer.img2 = some_new_image

Those are all nice to be able to do, but I don't think that the lack of them significantly impedes utility here. So given that it would be a decent lift to turn this into a widget I'd suggest not doing it unless you wanted to learn how to make a widget (which is a valuable skill).

jupyter_compare_view/compare.py Outdated Show resolved Hide resolved
jupyter_compare_view/compare.py Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Comment on lines 92 to 101
Args:
add_controls: pass False to not create controls
start_mode: either "circle", "horizonal" or "vertical"
circumference_fraction: size of circle outline as fraction of image width or height (whatever is bigger)
circle_size: the radius in pixel
circle_fraction: a fraction of the image width or height (whichever is bigger—called max_size in this document)
show_circle: draw line around circle
slider_time: time slider takes to reach clicked location
start_slider_pos: 0.0 -> left; 1.0 -> right
show_slider: draw line at slider
Copy link

Choose a reason for hiding this comment

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

Note that several parameters are not included in the docstring. And the return value is missing. Also it may be worth considering following the numpydoc style for consistentcy with the larger ecosystem of scientific and datascience packages.

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 added some missing parameters in the docstring. There is also a type hint for the return value. I think the current Google style docstring is fine, it's also widely used.

This was referenced Dec 6, 2022
Copy link
Member

@kolibril13 kolibril13 left a comment

Choose a reason for hiding this comment

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

Great work!
As soon as the conversations are resolved, this PR can be merged

amorgun and others added 2 commits December 6, 2022 21:51
Co-authored-by: Jan-Hendrik Müller <44469195+kolibril13@users.noreply.github.com>
* Update compare.py

* Update example_notebook

* Created using Colaboratory

* Update README.md
Copy link
Member

@christopher-besch christopher-besch left a comment

Choose a reason for hiding this comment

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

This is a very clean implementation in my opinion! I especially like that you're using types so frequently. Now we finally have a proper config-marshalling layer from Python to TypeScript. I really don't have anything to complain about, merge.

Thank you for your efforts!

@kolibril13 kolibril13 merged commit 3606514 into Octoframes:main Dec 9, 2022
@kolibril13
Copy link
Member

Thank you all for making this possible!
I will have a look at #43 later today and see what can be done with the version requirements.
After that, I ll create the next release☺️

@kolibril13
Copy link
Member

I just made the next release, @amorgun :
Do you have a Twitter account? If yes, can you give me your Twitter handle? Then I can make an announcement Tweet, with attribution to you.

Also, can you test this in google coolab?
Btw. google coolab just updated to python 3.8: https://colab.research.google.com/notebooks/relnotes.ipynb#scrollTo=wAN8otMuxpw3

@amorgun
Copy link
Contributor Author

amorgun commented Dec 10, 2022

Sure, my twitter is https://twitter.com/amorgun2.
I tested it in colab and it works fine. You can click "Open in Colab" badge in the readme, press "Runtime->Run all" and everything works.
I noticed that colab updated its python version but it was after #42 was merged.

@kolibril13
Copy link
Member

kolibril13 commented Dec 10, 2022

Cool!
I will send the tweet in a few days :)

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

4 participants