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

Standalone python scripts for canned analysis #153

Open
wants to merge 45 commits into
base: develop
Choose a base branch
from

Conversation

slabasan
Copy link
Collaborator

@slabasan slabasan commented Apr 23, 2024

  • remove examples/python_scripts/.gitignore once a python file has been added

@Thionazin Once you have your python scripts of different analyses, please add them to this PR.

@slabasan slabasan added area-docs Issues and PRs related to Thicket's documentation, notebooks, and examples priority-normal Normal priority issues and PRs status-work-in-progress PR is currently being worked on labels Apr 23, 2024
@pearce8 pearce8 removed their assignment May 1, 2024
@pearce8 pearce8 added the type-feature Requests for new features or PRs which implement new features label May 1, 2024
@pearce8 pearce8 changed the title WIP add examples directory and initial docs Standalone python scripts for canned analysis May 8, 2024
@pearce8 pearce8 added the status-help-wanted Issues which need a programmer to address them label May 8, 2024
@pearce8
Copy link
Collaborator

pearce8 commented May 17, 2024

@Thionazin
[ ] Address comments on PR
[ ] Make input parameter names clear, make clear which are optional, add --help messages
[ ] Add error checking for parameters
[ ] make_graph should be renamed to make_figure, also take optional parameters for title and label names
[ ] Upload new figures here

docs/examples.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@michaelmckinsey1 michaelmckinsey1 left a comment

Choose a reason for hiding this comment

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

  1. I think we rename examples.rst to canned_analysis.rst and change the doc title to Canned Analysis
  2. Is the examples directory going to contain other things than python scripts? Otherwise can we remove the python_scripts directory level and maybe these scripts are labeled like the tutorial notebooks 01A_display_valid_metadata_metric.py, 01B_stacked_line_charts.py.

examples/python_scripts/display_valid_metadata_metric.py Outdated Show resolved Hide resolved
examples/python_scripts/display_valid_metadata_metric.py Outdated Show resolved Hide resolved
examples/python_scripts/stacked_line_graphs_deepcopy.py Outdated Show resolved Hide resolved
examples/python_scripts/stacked_line_charts.py Outdated Show resolved Hide resolved
examples/python_scripts/stacked_line_charts.py Outdated Show resolved Hide resolved
docs/images/perc.png Outdated Show resolved Hide resolved
examples/python_scripts/stacked_line_charts.py Outdated Show resolved Hide resolved
examples/python_scripts/stacked_line_charts.py Outdated Show resolved Hide resolved
@pearce8 pearce8 requested a review from dyokelson July 29, 2024 16:44
@dyokelson
Copy link
Collaborator

dyokelson commented Jul 31, 2024

@Thionazin your unit tests are failing on flake8 formatting, make sure you run flake8 and check any of your files that changed back in, all checks/unit tests need to pass before we can merge. But I have other changes to suggest so this can wait until you've made other updates.

docs/examples.rst Outdated Show resolved Hide resolved
docs/examples.rst Outdated Show resolved Hide resolved
docs/examples.rst Outdated Show resolved Hide resolved
docs/examples.rst Outdated Show resolved Hide resolved
docs/examples.rst Outdated Show resolved Hide resolved
docs/examples.rst Outdated Show resolved Hide resolved
docs/images/perc.png Outdated Show resolved Hide resolved
examples/python_scripts/stacked_line_graphs_deepcopy.py Outdated Show resolved Hide resolved
@pearce8 pearce8 marked this pull request as ready for review August 2, 2024 15:04
Copy link
Collaborator

@dyokelson dyokelson left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @Thionazin, this is looking pretty good to me, just a question for @pearce8 about the strong scaling graph. Also @michaelmckinsey1 can you just go over your comments and let me know if any still apply or otherwise mark them as resolved?

Once those two things are settled I'll approve it for merging.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pearce8 Do these strong scaling results look as expected to you? Shouldn't we be seeing speedup for higher mpi world size? Just wanted to sanity check, in case I'm just missing something. The weak scaling graph looks as I would expect.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the problem size was not large enough?

@michaelmckinsey1
Copy link
Collaborator

[ ✓] Address comments on PR
[ ✓] Make input parameter names clear, make clear which are optional, add --help messages
[ ✓] Add error checking for parameters
[ ✓] make_graph should be renamed to make_figure, also take optional parameters for title and label names
[ ✓] Upload new figures here

slabasan and others added 2 commits October 10, 2024 17:01
- remove examples/python_scripts/.gitignore once a python file has been added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-docs Issues and PRs related to Thicket's documentation, notebooks, and examples priority-normal Normal priority issues and PRs status-help-wanted Issues which need a programmer to address them status-work-in-progress PR is currently being worked on type-feature Requests for new features or PRs which implement new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants