-
Notifications
You must be signed in to change notification settings - Fork 50
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 linear model #551
Ipynb linear model #551
Conversation
Thank you for opening this PR. Each PR into dev requires a code review. For the code review, look at the following:
|
Benchmarking Results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Just a few wording suggestions. I really like how you set up the LinearModel structure with thorough explanation at the top. Very clear.
Benchmarking Results [Update]
To:
|
There was a problem hiding this 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 small suggestions
" states = ['x', 'v']\n", | ||
" outputs = ['x']\n", | ||
" \n", | ||
" A = np.array([[0, 1], [0, 0]])\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add some more description of why these are the right values in the markdown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a more general description of what these values are supposed to represent near the top where we originally defined these values.
Codecov Report
@@ Coverage Diff @@
## dev #551 +/- ##
=======================================
Coverage 84.50% 84.50%
=======================================
Files 45 45
Lines 3504 3504
=======================================
Hits 2961 2961
Misses 543 543 |
Co-authored-by: Katy Jarvis <[email protected]> Co-authored-by: Christopher Teubert <[email protected]>
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more comments, but I'm approving the PR. Looks good!
examples/linear_model.ipynb
Outdated
"def future_loading(t, x=None):\n", | ||
" return m.InputContainer({})\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since future loading isn't required anymore, should we consider deleting this (and the above description of it) to reduce confusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And adding a note describing how we don't need a future loading equation because the model has no inputs
Benchmarking Results [Update]
To:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments.
Note: there are also still unresolved comments from Katy and my last review
examples/linear_model.ipynb
Outdated
" default_parameters = {\n", | ||
" 'thrower_height': 1.83,\n", | ||
" 'throwing_speed': 40,\n", | ||
" 'g': -9.81\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually want to have g as a property, since it doesn't change anything (the value is hard coded in E)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I remove the value 'g', in a later cell where we are creating the future loading function and are simulating to threshold, I receive an error saying: KeyError: 'g'.
examples/linear_model.ipynb
Outdated
"def future_loading(t, x=None):\n", | ||
" return m.InputContainer({})\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And adding a note describing how we don't need a future loading equation because the model has no inputs
Co-authored-by: Katy Jarvis <[email protected]>
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few last recommended changes, then it should be ready
Co-authored-by: Christopher Teubert <[email protected]>
Co-authored-by: Christopher Teubert <[email protected]>
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
Moved to nasa/progpy#70 |
Jupyter Notebook for Linear Model Example.