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

feat: change plotly figure widget to allow image annotation #285

Conversation

itepifanio
Copy link
Contributor

Closes #249

@itepifanio itepifanio changed the title Change plotly figure widget to allow image annotation feat: change plotly figure widget to allow image annotation Sep 11, 2023
Copy link
Contributor

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

Hi Italo,

thanks for your PR, really nice work. Mostly small comments, nothing major. I like the refactor you've done on the on_points_callback 👍

Regards,

Maarten


def update_data():
fig_widget: FigureWidget = solara.get_widget(fig_element)
fig_widget = solara.get_widget(fig_element)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the typing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing, it wasn't suppose to be removed

Comment on lines 306 to 309
if hasattr(fig_widget, 'data'):
length = len(fig_widget.data)
data = list(fig_widget.data)
fig_widget.data = data[length:]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you make this change? When is data not an attribute?

I think in any case, it's not a related change. I prefer you revert this, open a new PR, to avoid getting the PR stuck on unrelated things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted. Some changes I made in my codebase was added here by mistake, I was testing some things on the configuration. Sorry about that.

solara/website/pages/examples/visualization/annotator.py Outdated Show resolved Hide resolved
solara/website/pages/examples/visualization/annotator.py Outdated Show resolved Hide resolved
solara/website/pages/examples/visualization/annotator.py Outdated Show resolved Hide resolved
import solara

title = "Plotly Image Annotator"
text = solara.reactive('Draw on canvas')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's cleaner to do sth like:

shapes = solara.reactive(None)
...

        if "shapes" in relayout_data:
            shapes.value = relayout_data["shapes"]
...

if not shapes.value:
   solara.Markdown("## Draw on the canvas")
else:
  solara.Markdown("## Data returned by drawing")
  formatted_shapes = str(json.dumps(relayout_data["shapes"], indent=2, cls=CustomEncoder))
  solara.Preformatted(formatted_shapes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@maartenbreddels maartenbreddels force-pushed the issue-249-plotly-figure-for-image-annotation branch from b71aeff to a4db7ad Compare September 29, 2023 19:28
Copy link
Contributor

@maartenbreddels maartenbreddels 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 for the changes.
I see two files which are removed, please add them back, and please take a look at the comments I gave.
Hope we can get this in soon!

@@ -95,7 +96,7 @@ def Layout(children=[]):
sub_title="Solara helps you build powerful & scalable Jupyter and web apps <b>faster</b> and <b>easier</b>.",
button_text="Quickstart",
)

"""
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this needed? Did you have trouble running this part? Is there something can do about it?
In any case, for this PR I think you want to revert this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maartenbreddels yes, I had issues running the documentation* and ended up removing this files to run the documentation. This wasn't suppose to be deleted, it should be revered now.

*I followed the setup docs but had problems running the tests and docs. I'll try to setup again later and open some issues if needed.

Comment on lines 17 to 22
class CustomEncoder(json.JSONEncoder):
def default(self, o):
# Convert object instances to their string representation
if isinstance(o, object):
return str(o)
return super().default(o)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? Does plotly give back some objects? If so, it would be good to explain this in this piece of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does plotly give back some objects?

Yes, I added more explanation about it.

@maartenbreddels maartenbreddels merged commit 805da82 into widgetti:master Oct 6, 2023
12 checks passed
@maartenbreddels
Copy link
Contributor

Did some small changes, such as making the annotation mode the default.

Thank you a lot! ❤️

@itepifanio itepifanio deleted the issue-249-plotly-figure-for-image-annotation branch October 6, 2023 12:34
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.

Plotly Figure for Image Annotations
2 participants