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

Fix: update Jupyter kernel send function #304

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

fjebaker
Copy link
Contributor

Fixes an issue @inaki-ortizdelandaluce opened on a Dockerfile wrapper I maintain: fjebaker/cadabra-resources#1

The Server API was updated some months ago, but not in the Jupyter kernel, leading to runtime errors.

I have been planning to revisit the Jupyter kernel, modernize it, and flesh out the features I never got round to implementing, but I am yet to find the time... hopefully soon!

The `Server.send` function has been updated in much of the codebase to
include the `cell_id` argument, however this was not modified in the
Jupyter kernel. This lead to a Python error when trying to display the
result of a computation in a Jupyter notebook.
@kpeeters
Copy link
Owner

kpeeters commented Aug 2, 2024

Thanks. That change was made to enable users to change existing output cells; an example is in examples/ref_dynamical_updates.cnb. In essence, whenever you call display it will return the id of the output cell, and you can then update that output cell by calling display with the cell_id as parameter. The following block shows a single output cell with an updating counter (instead of 100 output cells):

out=0
for i in range(100):
   out=display(i, cell_id=out)

I think Jupyter can do this too, since version 5.1 of the Jupyter messaging protocol, with the update_display_data message type, as per https://jupyter-protocol.readthedocs.io/en/latest/messaging.html. This may be interesting to add to the Cadabra kernel.

@kpeeters kpeeters merged commit b686ebf into kpeeters:master Aug 2, 2024
@fjebaker
Copy link
Contributor Author

fjebaker commented Aug 2, 2024

Thanks for the explanation! And thanks for merging.

This may be interesting to add to the Cadabra kernel.

Agreed. I will keep that in mind when I attempt the rewrite!

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

2 participants