Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ImplictiEulerExtrapolation parallel #872
ImplictiEulerExtrapolation parallel #872
Changes from all commits
a2695f5
62195d2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
one improvement that here is just to make the first ones in the arrays equal, i.e.
u_tmps[1] = u_tmp
. Then the algorithm would be fine. Right now you have an extra thing for each one, so you have an extra Jacobian, and that's really bad memory-wise.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.
this is just silly. Why not just pass in
W
into these routines? It would fix these problems.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.
@kanav99 @huanglangwen this is something we should follow up on. It's because our original
calc_W!
was never intended for thread-safety, and so to make it thread-safe there is this code duplication. We should assume this case can happen in the next refactor.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.
Does it mean
calc_W!
should have an additional parameter of readonlyW
?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.
it should have
J
andW
I think, where it writes intoW
for a givenJ
, instead of pulling them from the cache (since there may be more than 1)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.
But it won't work on OOP.
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.
For OOP it will need to return W.
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 should have a pseudo-nlsolver kind of thing for algorithms which don't have an actual nlsolver like Rosenbrocks, the struct should have aliases to the
J
andW
and similar stuff, should be<: NLSolver
so that we have a common implementation of the functions. This way it won't make us change the original derivative utilities everytime we make a new algorithm just for an overload.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.
Note that algorithms with nlsolver struct are already well handled for parallel applications.
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.
why Romberg?