Skip to content

Refinements to key live script demo - #91

Draft
vijayiyer05 wants to merge 5 commits into
MATLAB-Community-Toolboxes-at-INCF:mainfrom
vijayiyer05:main
Draft

Refinements to key live script demo#91
vijayiyer05 wants to merge 5 commits into
MATLAB-Community-Toolboxes-at-INCF:mainfrom
vijayiyer05:main

Conversation

@vijayiyer05

Copy link
Copy Markdown
Collaborator

Converted the key demo live script (tiny_ophys_inference_detailed) to plain-text Live Code format.

Applied suggested small refinements from a short design review by Deep Learning Toolbox team. Thanks to @meghendra.

Notable learning from the review: the current approach of porting Keras models to TensorFlow (done in +interop) may not be needed for future zoo models published by originator in Keras, given the new importNetworkFromKeras function which supports TensorFlow 2.16+.

Apply suggested changes by Dev design reviewer wrt the importNetworkFromTensorFlow code section
Used to avoid warning message due to disallowed hyphen in current model zoo filenames
Was accidentally committed to parent folder of examples\ at prior iteration
@stevevanhooser

Copy link
Copy Markdown
Collaborator

Thanks @vijayiyer05 — and thanks to @meghendra for the review feedback baked in
here. Noting that I've read through this so it isn't sitting unattended.

I like the direction, particularly the move to plain-text Live Code. Being able
to diff and merge the narrative and the code in plain git, rather than only
through the MATLAB Comparison Tool, is a real win for a repo like this one.

A few things I'd like to settle before it comes out of draft — mostly so we know
which are yours and which are mine:

  1. Does the .mlx stay? As it stands this adds
    examples/tiny_ophys_inference_detailed.m alongside the existing .mlx. Two
    copies of the flagship demo will drift apart. Replace, or keep both through a
    transition?

  2. README and File Exchange links. The "View on File Exchange" and "Open in
    MATLAB Online" badges still point at tiny_ophys_inference_detailed.mlx. If
    the .m becomes canonical those should move with it — happy to take that on.
    Do you know whether File Exchange renders plain-text live code yet?

  3. Minimum MATLAB release. My understanding is the plain-text format is
    R2025a and later (setting aside the R2024b beta desktop). Opened in anything
    older the file still runs, but as a plain script — no formatted text,
    interactive controls, or stored output. We don't state a minimum release in
    the README today; if we adopt this format I think we should. Are you
    comfortable with R2025a as the floor for the examples?

  4. Your importNetworkFromKeras note is a good find and shouldn't stay buried in
    a PR description. Want me to open an issue to track possibly retiring the
    Keras→TensorFlow conversion in +interop?

No rush on any of it — happy to review properly whenever you mark it ready.

@vijayiyer05

Copy link
Copy Markdown
Collaborator Author

Thank you @stevevanhooser for the review. Quick replies:

  1. You read my mind, the transition should be made but meant for at least one point (say this PR) to carry both. Partly for posterity and partly to further validate Initial Full-Scale Inference Support #2.
  2. Have since confirmed that plain-text live code renders in the File Exchange viewer (yay!), so the link could be updated now or soon. Either way, this and maybe once more it seems good to scour the renders for both to check consistency before cutting the cord.
  3. Good point. Yes comfortable with R2025a as the minimum.
  4. An Issue sounds good. My PR comment was slightly equivocal since I'm not 100% sure the source model can be brought forward to the version that would allow the direct Keras pathway; so the Issue might also be suggestive/equivocal ("Consider...") unless you know better (which would be great!).

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.

2 participants