-
Notifications
You must be signed in to change notification settings - Fork 32
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
First step of generalizing the time domain integrator component to mphys builders #139
Conversation
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.
The changes look good and I am fine with merging this as is. One comment: do we want to include the mphys_time_derivative
tag in the documentation or skip it for now because its work in progress?
I wanted to test this feature but haven't gotten a chance yet. I can do the test after it is merged. |
I think we'll leave it out of the documentation for now. |
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.
The code changes look good to me. I just have a few comments on the readme file for the tests. I can also make these changes if you walk me through the details, @kejacobson.
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 think we can expand this explanation a bit. Reading it as an outsider to the time domain stuff, I had to do a few iterations to understand what is going on. I say the following items need clarification:
- In the first sentence, explain there are 2 versions of doing it. There are 3 paragraphs below, which confused me initially. We can include a brief explanation of what each method does in the first paragraph, and give the detailed explanation of what each directory contains below.
- We can expand the explanation for the two versions a bit more. Maybe highlight more differences between the two?
- The builder version explanation was a bit confusing. Is this an example class template for these models? Also you mention needing to replace the existing components with MELD, but if this example does work, to me, replacing it with MELD would just result in a different example. Can you explain this a bit more?
Progress on issue #8.