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.

Sign up for GitHub

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

Jump to bottom

Iss18 notebook plots #24

Merged
merged 16 commits into from
Jun 26, 2024
Merged

Iss18 notebook plots #24

merged 16 commits into from
Jun 26, 2024

Conversation

geoffwoollard
Copy link
Collaborator

#18

@geoffwoollard
Copy link
Collaborator Author

@DavidHerreros can you have a look at the notebook. I think it's ready to merge.

@DavidHerreros
Copy link
Collaborator

@DavidHerreros can you have a look at the notebook. I think it's ready to merge.

Sure! I will finish the cache funcitonality first and then change to this branch to test everything 👍

DavidHerreros
DavidHerreros requested changes Jun 25, 2024
View reviewed changes
Copy link
Collaborator

@DavidHerreros DavidHerreros left a comment

Choose a reason for hiding this comment

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

Suggestions for tutorial notebook 3.1:

  • Probably it would be useful to add an explanation of the tow paths that should be given to run the notebook? It is a bit hard to know where they should point to execute everything
  • I would also add the interactive path browser as implemented in the other notebooks

Suggestions for tutorial notebook 5:

  • It seems config_plotting.yaml is missing from the config file templates
  • I would implement the path browsing tool here as well, including a markdown cell specifing that there is a template available in the config_files folder

@geoffwoollard
Copy link
Collaborator Author

Regarding notebook 5, this is what an example config files looks like.

How do you suggest I interactively specify a way to make all those paths? Also the dist2dist_results.pkl_globs are strings with wildcards for glob to expand (like ls)


map2map_results:
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_0.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_1.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_2.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_3.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_4.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_5.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_6.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_7.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_8.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_9.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_10.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_11.pkl

dist2dist_results:
  pkl_fnames:
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_0.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_1.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_2.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_3.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_4.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_5.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_6.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_7.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_8.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_9.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_10.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_11.pkl
  pkl_globs:
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate[0-9]_norank_submission_*.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate[0-9][0-9]_norank_submission_*.pkl

@geoffwoollard
Copy link
Collaborator Author

geoffwoollard commented Jun 25, 2024 •
edited
Loading

Suggestions for tutorial notebook 3.1:

I didn't mean for this notebook to be on main. I've removed.

@DavidHerreros
Copy link
Collaborator

Regarding notebook 5, this is what an example config files looks like.

How do you suggest I interactively specify a way to make all those paths? Also the dist2dist_results.pkl_globs are strings with wildcards for glob to expand (like ls)


map2map_results:
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_0.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_1.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_2.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_3.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_4.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_5.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_6.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_7.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_8.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_9.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_10.pkl
  - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/analysis_results_20240415-npix224_submission_11.pkl

dist2dist_results:
  pkl_fnames:
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_0.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_1.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_2.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_3.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_4.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_5.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_6.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_7.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_8.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_9.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_10.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate10_norank_submission_11.pkl
  pkl_globs:
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate[0-9]_norank_submission_*.pkl
    - /mnt/home/gwoollard/ceph/repos/cryomethods_comparison_pipeline/results/distribution_to_distribution_20240416_npoolmicrostate[0-9][0-9]_norank_submission_*.pkl

I think with the new place holders in the config file is quite clear!

Only I have one question, pkl_globs and pkl_fnames are two different ways to specify the same paths? If that is the case, which one is prefered if they are specified at the same time? If only one of them is needed to run the notebook, I woould probably mention it in the config file template to make it clearer.

Also, while testing the notebook an import error is being raised on this line:

from cryomethods_comparison_pipeline.distribution_to_distribution import sort_by_transport

Any idea why this may be happeing? I have pulled the repo and installed everything again just in case but the error is still there

@geoffwoollard
Copy link
Collaborator Author

Only I have one question, pkl_globs and pkl_fnames are two different ways to specify the same paths? If that is the case, which one is prefered if they are specified at the same time? If only one of them is needed to run the notebook, I woould probably mention it in the config file template to make it clearer.

it's because there might be dozens of result .pkl files, and specifying them individually in the config file is tedious. In the notebook, some results are made from dist2dist_results.pkl_fnames, and others from pkl_fnames.pkl_globs. I could make the names more informative of the plots they are used for?

dist2dist_results:
  qopt_plot:
    pkl_fnames:
      -  ...
  emd_plot:
    pkl_globs: 
      - ...

@DavidHerreros
Copy link
Collaborator

Only I have one question, pkl_globs and pkl_fnames are two different ways to specify the same paths? If that is the case, which one is prefered if they are specified at the same time? If only one of them is needed to run the notebook, I woould probably mention it in the config file template to make it clearer.

it's because there might be dozens of result .pkl files, and specifying them individually in the config file is tedious. In the notebook, some results are made from dist2dist_results.pkl_fnames, and others from pkl_fnames.pkl_globs. I could make the names more informative of the plots they are used for?

dist2dist_results:
  qopt_plot:
    pkl_fnames:
      -  ...
  emd_plot:
    pkl_globs: 
      - ...

Oh ok now I get it!

I think the clarification would be useful :) Probably adding it as a comment to the config file would be enough so there is no need to change the code somewhere else.

@geoffwoollard
Copy link
Collaborator Author

Also, while testing the notebook an import error is being raised on this line:

from cryomethods_comparison_pipeline.distribution_to_distribution import sort_by_transport

Any idea why this may be happeing? I have pulled the repo and installed everything again just in case but the error is still there

Yes very good catch. That's an old repo I have locally - and I didn't realize I was importing from it. I've brought in the function to this repo now.

@geoffwoollard
Copy link
Collaborator Author

@DavidHerreros I've addressed your concerns. This should be good to merge. Can you confirm?

@DavidHerreros DavidHerreros self-requested a review June 26, 2024 15:01
@DavidHerreros
Copy link
Collaborator

@DavidHerreros I've addressed your concerns. This should be good to merge. Can you confirm?

Approved! 👍

@geoffwoollard geoffwoollard merged commit ff0d571 into main Jun 26, 2024
4 checks passed
@DSilva27 DSilva27 deleted the iss18_notebook_plots branch July 12, 2024 18:00
DSilva27 pushed a commit that referenced this pull request Aug 6, 2024
@geoffwoollard
Merge pull request #24 from flatironinstitute/iss18_notebook_plots
cba5051 
Iss18 notebook plots
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidHerreros DavidHerreros DavidHerreros approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@geoffwoollard @DavidHerreros