-
Notifications
You must be signed in to change notification settings - Fork 4
Adding solvers for quick experiments #21
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 15 19 +4
Lines 1505 2088 +583
==========================================
+ Hits 1505 2088 +583
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
brownbaerchen
left a comment
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.
Looks really nice. Lot's of new stuff. I have a few concerns about naming, which I put in the comments.
Of course I wasn't able to check very closely that the code is as clean and correct as possible with such a large PR, but overall cleanliness appears to be really good and the testing comprehensive, so that seems fine to me.
I am wondering a bit about the direction qmat overall evolves in. Its scope is expanding from a repository of quadrature coefficients to a playground for SDC. This is not necessarily bad. But I feel like most of your issues with pySDC stem from the code trying to be really flexible, really easy to understand and to support everything and invariably failing to excel at all of it. Of course the eierlegende Wollmilchsau doesn't exist. It's not pySDC and it will not be qmat. Already, I am confused why there is a specialized Dahlquist solver and an implementation of a "differential operator" for the Dahlquist problem...
Does it make sense to split the generation of
| def evalF(self, u, t, out): | ||
| # TODO : your implementation | ||
| pass | ||
| ``` |
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 this template is not entirely sufficient explanation. In particular because of some mixing of stuff.
If I understand correctly, the "differential operator" implements the evalF when to me it should be called only eval because the differential operator is already
I have not yet read the code this documents, but that is also the intended order for new contributers and I am pretty sure I would not be the only one to be confused.
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.
Agree on solving the confusion, I'll improve the template. But eval is already a built-in python function, and the name should echo with the fSolve method. We could change to fEval maybe ... but it feels a bit weird
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.
Well, how set are you on the "differential operator" name? I would personally change that and then the other names are fine.
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 didn't find anything better at the moment. Problem is too generic and looks too close to pySDC. And at the end, we do need a class representing
a.k.a
which I imagined can be named a (time) "differential operator". I also thought about ODERightHandSide, but it didn't felt as simple and concise as DiffOp
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 don't think problem is too generic. Yes pySDC has problems (pun intended) but so does the rest of the world. I think differential operator is also very generic, but I personally don't expect it to have properties like
I don't know what the perfect name here is, but maybe this is another sign that this belongs in a separate repository, not in the one that computes quadrature coefficients.
|
Thanks for the review, I really appreciate. I've left (for now) the main issue to solve, which is how to name a class allowing to evaluate
I'm still not sure about the |
|
Now on the concern of So it did degenerated a bit, but I believe
And the tutorials show how to do it in a naive (not efficient) way, while the provided implementation gives a generic & memory efficient approach (very similar to what was done in SWEET or for Dedalus). The solvers and DiffOp classes contribute then to point 2, and the potential experiments with it (hence the playgrounds) came naturally. Maybe we could write in the contributing guide some development conditions for the future, as :
Ideally, |
|
Just my 2 cents here: beware of scope creep. It happens and always starts like "yeah, just added this here and there, might be removed later anyway, what's the harm", you name it. Listen to the old man, been there, done that. There is no point having another I have no problem (hahaha) with a |
|
I fully agree with the wise man 😉 ... hence I don't think So I hope we'll find a better ground when those two directions intersect, and maybe make a better version of |
|
Looks like we should talk in Dresden about it. Maybe @brownbaerchen would like to give a talk about the future pf pySDC to set the stage? |
|
I fully understand why Martin does not want to use pySDC for easy tests. I agree that a code that is simpler than pySDC is not a bad idea. Generally, I think the contents of this PR are good. However, I really think the two goals for qmat you mention above are kind of unrelated and therefore would benefit from having their own repositories. In particular, the new playgrounds are not part of either goal. They are part of a third goal, which is new SDC research.
|
|
Ok, argument heard (and accepted 😉). So I could create an additional repository
In
If this looks good on your side, I would suggest the following steps :
Of course, step 1 and 2 could be done quickly, while 3, 4, and 5 would need a bit more time (and I would focus on it during my next train travel). What do you think ? |
This PR intends to bring
qmatto version 1.0.0, including many new features, in particular :DiffOpclass for non-linear problem to implement some toy problemsAll implemented solver classes (and associated tutorials) make use of the$Q$ and $Q_\Delta$ coefficients of
qmat, and allows to easily test different time-schemes on different problems.