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

Use argparse to add --output flag to plotting script #19

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

tkphd
Copy link
Contributor

@tkphd tkphd commented Jun 11, 2024

This PR (programmed by an alleged human) uses the argparse module to document the plot_terse_amdahl_results.py program, adding a --output flag (required), revoking the requirement of JPEG output (matplotlib will guess the appropriate format from the filename, and error out if unknown), and updates the lesson content to use the new flag.

Supersedes #17. Closes #17.

@tkphd tkphd requested a review from ocaisa June 11, 2024 19:41
Copy link

github-actions bot commented Jun 11, 2024

🆗 Pre-flight checks passed 😃

This pull request has been checked and contains no modified workflow files or spoofing.

Results of any additional workflows will appear here when they are done.

@tkphd tkphd requested a review from reid-a June 11, 2024 19:42
@ocaisa
Copy link
Contributor

ocaisa commented Jun 12, 2024

@tkphd This is blocked until we update our CI, see #18 . It needs an access token secret, but I do not have the powers to add one.

Copy link
Contributor

@ocaisa ocaisa left a comment

Choose a reason for hiding this comment

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

#17 also covered what happens if the output already exists? The --overwrite option seems like a good idea

episodes/files/plot_terse_amdahl_results.py Show resolved Hide resolved
episodes/files/plot_terse_amdahl_results.py Outdated Show resolved Hide resolved
@ocaisa
Copy link
Contributor

ocaisa commented Jun 12, 2024

Well that's frustrating, even the updated CI doesn't work

@tkphd tkphd force-pushed the script branch 2 times, most recently from 534e637 to f8d1da5 Compare June 12, 2024 18:51
Copy link
Contributor

@ocaisa ocaisa left a comment

Choose a reason for hiding this comment

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

I'm happy to merge this but until the CI is fixed it won't actually deploy

@tkphd
Copy link
Contributor Author

tkphd commented Jun 12, 2024

Nah, we'll get the CI working. It looks like Toby has been actively supporting related issues on the Slack #workbench; I'm working through his advice to others.

@tkphd
Copy link
Contributor Author

tkphd commented Jun 12, 2024

... or maybe it won't help, and merging this will allow us to try PRs that change the Actions instead.

@tkphd tkphd merged commit 836cb22 into carpentries-incubator:main Jun 12, 2024
2 of 3 checks passed
@tkphd tkphd deleted the script branch June 12, 2024 19:35
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