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

VitessceConfig.to_python method #115

Merged
merged 44 commits into from
Jan 23, 2022
Merged

Conversation

keller-mark
Copy link
Member

@keller-mark keller-mark commented Dec 3, 2021

From @mccalluc in the Spatial Omics Meeting Agenda:

The SDK is procedural: It takes a series of statements and assignments to temporary variables to create the desired viewconf. The object we have at the end gives us no way to get the sequence of operations necessary to create the object.
This makes idiomatic notebook generation difficult: All we can do is serialize to the JSON view-conf, but we've lost all the readability of the SDK

...

Three proposals, none of them easy:
• Walk the JSON viewconf, and try to identify substructures that can be created with the SDK.
• In the portal-ui code, instead of using the SDK directly, use something like the Command Pattern to build up the sequence of statements that can either be executed, or dropped into the notebook.
• Move the SDK from a procedural to a functional style, so that the repr for an object is executable code that will reproduce the object.

I wrote my understanding of the problem in the meeting agenda document. Building on your PR #66, in this PR I added a .to_python method for VitessceConfig instances. Running vc.to_python generates Python code that can be used to reconstruct the view config. It is kind of a hybrid of proposals 1 and 3 above. I added an example notebook at docs/notebooks/config_to_python.ipynb. Would this solution work for the notebook generation use case?

Also:
Fixes #63
Fixes #54
Fixes #62

Copy link
Contributor

@mccalluc mccalluc left a comment

Choose a reason for hiding this comment

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

Lots of comments on the trees, not really understanding the forest.

Rather than test on the structure of the reconstructed_vc_dict, I'd be interested to see a check on the actual output of to_python.

Did you consider building the python as a syntax tree, rather than as a string? It would be harder, no doubt, but it would give us a better guarantee that the structure is good.

tests/test_config.py Outdated Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/repr.py Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
").set_coordination_value(\n",
" c_type=\"spatialTargetY\", c_scope=\"A\", c_value=-900\n",
").add_view(\n",
" dataset=\"dries-2019\",\n",
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 would be good to clarify with Nils what precisely he wants from the generated node that the json doesn't offer. One thing he might be looking for is avoiding cases like this where the same uuid string is incorporated as multiple points in the code... which might be tricky. Or maybe it's enough to get it into the python syntax and avoid the extra quotes around everything.

@mccalluc
Copy link
Contributor

mccalluc commented Dec 3, 2021

Also: This is awesome! Thank you so much for taking it on.

vitessce/repr.py Outdated Show resolved Hide resolved
vitessce/repr.py Outdated Show resolved Hide resolved
@mccalluc mccalluc mentioned this pull request Dec 6, 2021
keller-mark and others added 7 commits December 7, 2021 09:42
Co-authored-by: Chuck McCallum <mccalluc@users.noreply.github.com>
Co-authored-by: Chuck McCallum <mccalluc@users.noreply.github.com>
Co-authored-by: Chuck McCallum <mccalluc@users.noreply.github.com>
Co-authored-by: Chuck McCallum <mccalluc@users.noreply.github.com>
Co-authored-by: Chuck McCallum <mccalluc@users.noreply.github.com>
Copy link
Contributor

@mccalluc mccalluc left a comment

Choose a reason for hiding this comment

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

More thoughts!

vitessce/config.py Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/config.py Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
vitessce/config.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants