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

Ipynb benchmarking #2

Merged
merged 10 commits into from
Aug 11, 2023
Merged

Ipynb benchmarking #2

merged 10 commits into from
Aug 11, 2023

Conversation

aqitya
Copy link
Contributor

@aqitya aqitya commented Jul 6, 2023

Jupyter Notebook for Benchmarking Example.

@aqitya aqitya requested a review from teubert July 6, 2023 17:50
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for your contributions. If you haven't already, please send a signed Contributor License Agreement (CLA) to Christopher Teubert ([email protected]). CLAs can be found here: https://github.com/nasa/prog_models/tree/master/forms. Also, make sure you're familiar with the developer notes and contributing sections of our developers guide, https://nasa.github.io/progpy/dev_guide.html#notes-for-developers

@github-actions
Copy link

github-actions bot commented Jul 6, 2023

Thank you for opening this PR. Each PR into dev requires a code review. For the code review, look at the following:

  • Reviewer (someone other than author) should look for bugs, efficiency, readability, testing, and coverage in examples (if relevant).
  • Ensure that each PR adding a new feature should include a test verifying that feature.
  • All errors from static analysis must be resolved.
  • Review the test coverage reports (if there is a change) - will be added as comment on PR if there is a change
  • Review the software benchmarking results (if there is a change) - will be added as comment on PR
  • Any added dependencies are included in requirements.txt, setup.py, and dev_guide.rst (this document)
  • All warnings from static analysis must be reviewed and resolved - if deemed appropriate.

@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2023

Codecov Report

Merging #2 (5309a9b) into dev (334b190) will not change coverage.
Report is 19 commits behind head on dev.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev       #2   +/-   ##
=======================================
  Coverage   84.17%   84.17%           
=======================================
  Files          99       99           
  Lines       10106    10106           
=======================================
  Hits         8507     8507           
  Misses       1599     1599           

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@aqitya aqitya requested a review from kjjarvis July 7, 2023 20:44
Copy link
Contributor

@kjjarvis kjjarvis left a comment

Choose a reason for hiding this comment

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

Nice start! This looks very good. Here are a few comments, and we'll also need to address the extra files (I think Chris said he'd help with this).

"metadata": {},
"outputs": [],
"source": [
"%matplotlib inline"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to show up at the top? (the answer might be yes, I'm not very familiar enough with jupyter notebooks)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typically, no, but I think with how we are formatting the Jupyter Notebooks, we want that cell there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember us talking about it at one point in the past, but I can't remember why it's here. Can you explain it for me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We mentioned how the automated notebooks had this at the top. But from what I can tell, this does not serve a purpose for us, so I'll remove this cell.

examples/benchmarking.ipynb Outdated Show resolved Hide resolved
examples/benchmarking.ipynb Outdated Show resolved Hide resolved
examples/benchmarking.ipynb Outdated Show resolved Hide resolved
examples/benchmarking.ipynb Outdated Show resolved Hide resolved
examples/benchmarking.ipynb Outdated Show resolved Hide resolved
examples/benchmarking.ipynb Outdated Show resolved Hide resolved
examples/benchmarking.ipynb Outdated Show resolved Hide resolved
examples/benchmarking.ipynb Outdated Show resolved Hide resolved
@aqitya aqitya requested a review from kjjarvis July 13, 2023 09:01
Copy link
Contributor

@kjjarvis kjjarvis left a comment

Choose a reason for hiding this comment

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

@aqitya there are still some outstanding comments from the last review that need to be addressed. Please take a look, and let me know if you don't agree and we can brainstorm solutions together.

examples/benchmarking.ipynb Outdated Show resolved Hide resolved
examples/benchmarking.ipynb Outdated Show resolved Hide resolved
examples/benchmarking.ipynb Outdated Show resolved Hide resolved
examples/benchmarking.ipynb Outdated Show resolved Hide resolved
examples/benchmarking.ipynb Outdated Show resolved Hide resolved
examples/benchmarking.ipynb Outdated Show resolved Hide resolved
Copy link
Contributor

@teubert teubert left a comment

Choose a reason for hiding this comment

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

Looking good. A few suggestions

@aqitya aqitya requested review from teubert and kjjarvis August 9, 2023 21:12
Copy link
Contributor

@kjjarvis kjjarvis left a comment

Choose a reason for hiding this comment

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

This notebook is looking great, approved. One idea for the future - now that ProgPy is combined, we have two 'benchmarking' examples. Should we rename these files with something more specific for clarity?

Copy link
Contributor

@teubert teubert left a comment

Choose a reason for hiding this comment

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

Looks great. Good work!

@teubert teubert merged commit b9e3ff5 into dev Aug 11, 2023
27 of 28 checks passed
@teubert teubert deleted the ipynb-benchmarking branch August 16, 2023 22:09
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.

4 participants