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

Mccalluc/serve notebooks #1873

Merged
merged 12 commits into from
Jun 7, 2021
Merged

Mccalluc/serve notebooks #1873

merged 12 commits into from
Jun 7, 2021

Conversation

mccalluc
Copy link
Contributor

Towards #1605. @ilan-gold , considered as a proof of concept, is this much reasonable? If it's easy for you, do you want to fill out the TODO with the rendering command? (This is syntactically valid, but I have not actually started up a kernel and tried to run it.)

@mccalluc mccalluc requested a review from ilan-gold May 27, 2021 21:23
@ilan-gold
Copy link
Member

@mccalluc There appears to be a bug in vitessce-python with to_dict of some sort

@ilan-gold
Copy link
Member

See vitessce/vitessce-python#63

@ilan-gold
Copy link
Member

ilan-gold commented May 28, 2021

We should also add some sort of validation for this to Vitessce, the schema is not strong enough, but may also not be the right place to do such validation

@ilan-gold
Copy link
Member

ilan-gold commented May 28, 2021

And in Vitessce: vitessce/vitessce#952

@mccalluc
Copy link
Contributor Author

@ilan-gold :

  • Thanks for filling out the notebook!
  • Bigger picture, do you share the sense that this is something Nils wants, and it makes sense to complete this much, merge, and get feedback... and later we can expose it in the UI? (That is PR Mccalluc/UI for serve notebooks #1874.)
  • Am I interpreting the situation correctly that those issues are blockers?
  • Style: maybe combine the python into one cell? Or do smaller chunks make sense?
  • Content: We could inject more metadata, or more instructions... but maybe what's there is fine?

Do you want to see this through to completion? Or would you prefer that it stay with me?

@ilan-gold
Copy link
Member

* Bigger picture, do you share the sense that this is something Nils wants, and it makes sense to complete this much, merge, and get feedback... and later we can expose it in the UI? (That is PR #1874.)

Really do not know for sure, but I think that plan makes sense - do this PR then get feedback on the (functional) notebook output.

* Am I interpreting the situation correctly that those issues are blockers?

Indeed :(

* Style: maybe combine the python into one cell? Or do smaller chunks make sense?

No preference really - I think we have it separated for room for comments (check out some of the notebooks at https://vitessce.github.io/vitessce-python/ for reference)

* Content: We could inject more metadata, or more instructions... but maybe what's there is fine?

I was thinking about this too - I think we can wait for Nils' feedback.

Do you want to see this through to completion? Or would you prefer that it stay with me?

I do not have a preference, but the 3D stuff + ongoing bitmask/Cytokit work takes precedence due to the upcoming data releases so this could languish for a while if given to me. More likely though is that I just won't have the bandwidth to keep up with the back-and-forth feedback loop rather than the actual engineering.

@mccalluc
Copy link
Contributor Author

mccalluc commented May 28, 2021

@ilan-gold : Following from the conversation at the meeting, I can make a branch that tries to add more explanation into the viewconf classes? I'm not at all sure that the experiment will be successful, but it's something I could do... and it would also let me get more familiarity with the view conf generation.

So: Nothing for you to do right now.

@keller-mark notes:

The python package is currently missing any kind of “getters”. For instance, once you add a view / dataset to the view config, there are no methods to get view / dataset instances and update / manipulate it. If we had those, you could start from the JSON -> create VitessceConfig instance -> run a method like .get_dataset(“CODEX”) -> AnnData / NumPy / other python-native objects

@mccalluc
Copy link
Contributor Author

mccalluc commented Jun 3, 2021

@ilan-gold -- Could you approve this much? After this, I would do a bit in assay_confs.py, and then vitessce-python. Exposing it in the UI would come last. (#1874)

@ilan-gold
Copy link
Member

Please do request my review once you are ready with the assay_confs so we can test it out with "real data"

@mccalluc mccalluc merged commit 9e7af62 into master Jun 7, 2021
@mccalluc mccalluc deleted the mccalluc/serve-notebooks branch June 7, 2021 13:23
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