Have time steppers overwrite in update_u again #4748
Conversation
It turns out that it is sometimes difficult for the caller to determine the correct value to pass into the function. This largely reverts bf4fd0a.
|
Sorry, but it looks to me like this PR does not fix the dense output bug #4399. I tried running a BBH with the 3-index constraint set to zero at t=0. I used this branch at 3 different revisions: , and one of the following: 1: nothing (no additional commit) — revision 9200dd5 I verified that spectre.out reported the correct revision in each case. At t=0, here's the L2 norm of the 3-index constraint for each case:
|
|
OK, try now. An old optimization was bypassing the fix when run at t=0. This fix is not really related to the update_u changes, so I'm going to leave it as a separate commit. |
| if ((coupling.local_end() - 1)->value() == time) { | ||
| return; |
There was a problem hiding this comment.
I think this means if we are at the most recent history time, just return (i.e. don't do dense output), but I'm not 100% certain. Could you add a comment?
There was a problem hiding this comment.
You are correct. I've added a comment.
wthrowe
force-pushed
the
fix_dense_output
branch
from
b9ba0e5
to
a1e166d
Compare
There was a problem hiding this comment.
I'm happy with the changes, but @geoffrey4444 should confirm again that these changes now fix the LTS dense output bug
I will try this again. Thanks! |
There was a problem hiding this comment.
I did not review the code line by line but did try this again, and now the dense and not-dense output for the 3-index constraints agree, even at t=0.
Without this fix, the constraints are not zeroed correctly at t=0, and they level off to a larger value by time t=10 (though strangely even without the fix, I don't see the linear growth I saw back in November when I first spotted the symptoms of the bug this PR fixes).
I see small differences vs the hack fix, but I don't think they are significant.
So I'm happy to approve.
|
If you're doing filtering, I'm guessing the hack and this version interact with that differently. If not, I don't immediately see a reason they would differ, but I wouldn't worry about it for the moment. |
It turns out that it is sometimes difficult for the caller to determine the correct value to pass into the function.
This largely reverts bf4fd0a.
Fixes #4399
@geoffrey4444 This should remove the need for the dense output hack. Can you test this and verify that your results look good?
Proposed changes
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments