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

Improves demo scripts #34

Merged
merged 12 commits into from
Nov 3, 2023
Merged

Improves demo scripts #34

merged 12 commits into from
Nov 3, 2023

Conversation

neukym
Copy link
Member

@neukym neukym commented Oct 28, 2023

Fixes #24. Minor improvements for both the plotting demo, and the save/load demo, showing off more functionality than the originals.

@caiw - can you make sure you can run these yourself locally before accepting?

@neukym neukym added 📑 documentation Improvements or additions to documentation ready-for-merging labels Oct 28, 2023
@neukym neukym requested a review from caiw October 28, 2023 19:37
@neukym neukym linked an issue Oct 30, 2023 that may be closed by this pull request
Copy link
Member

@caiw caiw Nov 3, 2023

Choose a reason for hiding this comment

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

I don't seem to be able to add line-by-line comments on .ipynb files on Github. This file looks good to me except that the data file kymata_mirror_Q3_2023_expression_endtable.nkg is not included, so I can't actually run it.

Of course I have just fetched the file from Teams, so I can run the rest of the notebook, but I think we should discuss including/downloading data files (#1) before merging this in. (The referenced data file is, for example, 27 MB, though we have considered adding compression to the format: see #36).

Copy link
Member

Choose a reason for hiding this comment

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

This looks good, with the same caveat about relying on non-included data files.

I have a suggestion for this, but I can't do inline suggetions on notebook files (I think) so I will just make it and then pass the review back to @neukym.

@caiw
Copy link
Member

caiw commented Nov 3, 2023

Ok I can't request a review from @neukym but after you've informally reviewed my changes It's ready to merge.

@caiw caiw self-requested a review November 3, 2023 11:36
@caiw caiw added ⏸️ blocked/on-hold Blocked or otherwise waiting for something and removed ready-for-merging labels Nov 3, 2023
@caiw
Copy link
Member

caiw commented Nov 3, 2023

@neukym, the new changes include changes from PR #41, so that should be reviewed first.

@caiw caiw added ready-for-merging and removed ⏸️ blocked/on-hold Blocked or otherwise waiting for something labels Nov 3, 2023
@neukym neukym merged commit b9c4719 into main Nov 3, 2023
1 check passed
@neukym neukym deleted the improve-existing-demos branch November 3, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📑 documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider converting tutorials/demos to Jupyter notebooks Add tutorial for plotting expression plots
2 participants