#1633: backport MoL revision 225 --------------------------+------------------------------------------------- Reporter: rhaas | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: Other | Version: development version Keywords: MoL backport | --------------------------+------------------------------------------------- Before MoL did not initialize the RHS grid functions of the slow evolved variables to zero (but GRHydro did explicitly so this never showed up).
Revision 225 adds the initialization.
#1633: backport MoL revision 225 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: review Priority: minor | Milestone: Component: Other | Version: development version Resolution: | Keywords: MoL backport ---------------------+------------------------------------------------------ Changes (by rhaas):
* status: new => review
#1633: backport MoL revision 225 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: review Priority: minor | Milestone: Component: Other | Version: development version Resolution: | Keywords: MoL backport ---------------------+------------------------------------------------------
Comment (by knarf):
Why is MoLDebug used here? If this triggers, the code aborts anyway. I don't see harm in outputting too much information in that case. Also, please make the message a bit more "official". "Arg" is cute and all, but maybe you could think of something better? :)
Could CCTK_ERROR be used instead of CCTK_WARN(0, ?
#1633: backport MoL revision 225 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: review Priority: minor | Milestone: Component: Other | Version: development version Resolution: | Keywords: MoL backport ---------------------+------------------------------------------------------
Comment (by rhaas):
All of these are sound suggestions. However:
The code fragment is a copy of the lines already in Init_RHS for the regular evolved variables. Hence the names and debug output. I would rather not change names/WARN<->ERROR/etc *only* for the slow sector and not eg also for normal RHS, array RHS, complex RHS etc.
As a matter of fact it would seem much more reasonable to me to rewrite all these routines that are almost identical between array evolved variables, slow evolved variables, evolved variables, complex evolved variables, complex array evolved variables (did I miss any?) such that they all use a common worker routine an a short individual wrapper. Right now MoL has very much code duplication which makes it very hard to change anything since one has to make almost the same change at many places.
#1633: backport MoL revision 225 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: review Priority: minor | Milestone: Component: Other | Version: development version Resolution: | Keywords: MoL backport ---------------------+------------------------------------------------------
Comment (by knarf):
Your argument makes a lot of sense, and I don't want to be the one holding you back from cleaning up either. :)
#1633: backport MoL revision 225 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: review Priority: minor | Milestone: Component: Other | Version: development version Resolution: | Keywords: MoL backport ---------------------+------------------------------------------------------
Comment (by hinder):
But not in the backport, right? The backport should just fix the wrong behaviour in as minimal a way as possible.
#1633: backport MoL revision 225 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: review Priority: minor | Milestone: Component: Other | Version: development version Resolution: | Keywords: MoL backport ---------------------+------------------------------------------------------
Comment (by rhaas):
I would still apply just the proposed patch to the release version. Any cleanup will happen (eventually, I was waiting for the IMEX stuff to come in but that seems stalled) in trunk.
#1633: backport MoL revision 225 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: review Priority: minor | Milestone: Component: Other | Version: development version Resolution: | Keywords: MoL backport ---------------------+------------------------------------------------------
Comment (by hinder):
Has the fix been backported to the released version?
#1633: backport MoL revision 225 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: review Priority: minor | Milestone: Component: Other | Version: development version Resolution: | Keywords: MoL backport ---------------------+------------------------------------------------------
Comment (by anonymous):
The fix has not been backported. It is waiting for a "Please apply". This will also require some follow up patch to the slow evolved test in GRHydro though (since the current slow evolution code in MoL actually relies on the RHS not being cleared...)
#1633: backport MoL revision 225 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: reopened Priority: minor | Milestone: Component: Other | Version: development version Resolution: | Keywords: MoL backport ---------------------+------------------------------------------------------ Changes (by hinder):
* status: review => reopened
Comment:
So if the followup patch is not implemented, the test would break? Is that just a problem with the test, or with the code? And did you mean MoL in comment 8, or GRHydro? In either case, I think we should wait until the followup patch is ready. I am changing the status to "reopened" as the followup patch is not yet available for review.
#1633: backport MoL revision 225 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: reopened Priority: minor | Milestone: Component: Other | Version: development version Resolution: | Keywords: MoL backport ---------------------+------------------------------------------------------
Comment (by rhaas):
Ah, never mind. No follow up is required for the RHS since only a different kind of multi-rate method was affected (namely the RK2-2-1 and RK4-2-1 methods but not the RK4-RK2 method used here).
#1633: backport MoL revision 225 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: closed Priority: minor | Milestone: Component: Other | Version: development version Resolution: fixed | Keywords: MoL backport ---------------------+------------------------------------------------------ Changes (by rhaas):
* status: reopened => closed * resolution: => fixed
Comment:
Committed in rev 227-230 of MoL and rev 11 of TestMoL.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit