Hi,
A change was made a few months ago to ADMBase to solve some problems with the ADMBase variables not being synchronised in some situations where they need to be. This was r59 in ADMBase SVN. The bit I'm worried about is this:
+# For Mesh Refinement it is possible (eg Cowling approximation) that ADMBase variables will need synchronization even when not evolved (eg when a new refined grid appears) +if (CCTK_Equals(evolution_method, "static") || CCTK_Equals(evolution_method, "none") || CCTK_Equals(lapse_evolution_method, "static") || CCTK_Equals(shift_evolution_method, "static") || CCTK_Equals(dtlapse_evolution_method, "static") || CCTK_Equals(dtshift_evolution_method, "static") ) +{ + SCHEDULE ADMBase_Boundaries IN MoL_PostStep BEFORE ADMBase_SetADMVars + { + LANG: C + OPTIONS: LEVEL + SYNC: lapse + SYNC: dtlapse + SYNC: shift + SYNC: dtshift + SYNC: metric + SYNC: curv + } "Select ADMBase boundary conditions - may be required for mesh refinement" +}
For any code which does not use the ADMBase::[lapse/shift]evolution_method parameters to control their evolution, unnecessary syncs will be performed for the ADMBase variables. Whereas before these parameters seemed to be a superfluous part of the ADMBase infrastructure (after all, why activate your evolution thorn if you're not going to evolve with it?), it now seems that it is essential to extend and use these parameters if you want to use ADMBase, or performance will suffer. The GT evolution code would suffer from this, the last time I looked at it, and other groups' codes might also.
Hi,
On Thu, Aug 18, 2011 at 12:04:45AM +0200, Ian Hinder wrote:
For any code which does not use the ADMBase::[lapse/shift]evolution_method parameters to control their evolution, unnecessary syncs will be performed for the ADMBase variables. Whereas before these parameters seemed to be a superfluous part of the ADMBase infrastructure (after all, why activate your evolution thorn if you're not going to evolve with it?), it now seems that it is essential to extend and use these parameters if you want to use ADMBase, or performance will suffer. The GT evolution code would suffer from this, the last time I looked at it, and other groups' codes might also.
I didn't think of that, right. I see two possible solutions: either do use the ADMBase parameters in all evolution thorns (you don't really have to use them - extend the parameter and set it to that in the parameter file - I know, that's ugly but it works); or we provide these syncs in an extra thorn or via a new parameter within ADMBase. Both approaches are not ideal imho. The ideal would be if each evolution thorn would actually use these parameters, but that is arguably a lot to request. Or, wouldn't the following work: evolution_method has 'none' and 'static' as meaning 'no evolution is done'. We could use 'none' for nothing at all is done and 'static' for real "static evolutions" with 'none' being the default. However, this would require 'none' being also provided for the other *_evolution_method parameters.
Frank
On Wed, Aug 17, 2011 at 10:09 PM, Frank Loeffler knarf@cct.lsu.edu wrote:
Hi,
On Thu, Aug 18, 2011 at 12:04:45AM +0200, Ian Hinder wrote:
For any code which does not use the ADMBase::[lapse/shift]evolution_method parameters to control their evolution, unnecessary syncs will be performed for the ADMBase variables. Whereas before these parameters seemed to be a superfluous part of the ADMBase infrastructure (after all, why activate your evolution thorn if you're not going to evolve with it?), it now seems that it is essential to extend and use these parameters if you want to use ADMBase, or performance will suffer. The GT evolution code would suffer from this, the last time I looked at it, and other groups' codes might also.
I didn't think of that, right. I see two possible solutions: either do use the ADMBase parameters in all evolution thorns (you don't really have to use them - extend the parameter and set it to that in the parameter file - I know, that's ugly but it works); or we provide these syncs in an extra thorn or via a new parameter within ADMBase. Both approaches are not ideal imho. The ideal would be if each evolution thorn would actually use these parameters, but that is arguably a lot to request.
I think that is not a lot to request -- ADMBase is the lingua franca that allows all Cactus relativity codes to interact, and it defines a certain standard that is documented, that was thought about hard, and was extensively discussed (publicly). It's not that difficult to use, and errors in a parameter file can be caught in a user thorn's paramcheck bin.
-erik
Or, wouldn't the following work: evolution_method has 'none' and 'static' as meaning 'no evolution is done'. We could use 'none' for nothing at all is done and 'static' for real "static evolutions" with 'none' being the default. However, this would require 'none' being also provided for the other *_evolution_method parameters.
Frank
Users mailing list Users@einsteintoolkit.org http://lists.einsteintoolkit.org/mailman/listinfo/users
Hello all,
alright, here's my two cents.
On Thu, Aug 18, 2011 at 12:04:45AM +0200, Ian Hinder wrote:
For any code which does not use the ADMBase::[lapse/shift]evolution_method parameters to control their evolution, unnecessary syncs will be performed for the ADMBase variables. Whereas before these parameters seemed to be a superfluous part of the ADMBase infrastructure (after all, why activate your evolution thorn if you're not going to evolve with it?), it now seems that it is essential to extend and use these parameters if you want to use ADMBase, or performance will suffer. The GT evolution code would suffer from this, the last time I looked at it, and other groups' codes might also.
We were affected and have now extended the ADMBase parameters in Kranc2BSSN. We don't use these parameters at all in the code otherwise, they are just set to something other than "none" to make ADMBase happy. And yes, it was an annoying change that required sending out an email to each and every user informing them about the change (and we still have issues where we forget). As far as testing for these things in paramcheck goes: Kranc does not provide this functionality, so one would have to write a hackish XXXHelper thorn the way McLachlan does. One could of course use the evolution method to select eg. the shift evolution method (harmonic, gamma driver etc) but that would break all of our existing parameter files. Also some parameters are simply never used, since eg. we do not even support and a dtlapse evolution method (I think, Ian will actually know better).
I think that is not a lot to request -- ADMBase is the lingua franca that allows all Cactus relativity codes to interact, and it defines a certain standard that is documented, that was thought about hard, and was extensively discussed (publicly). It's not that difficult to use, and errors in a parameter file can be caught in a user thorn's paramcheck bin.
Precisely because ADMBase is used by everyone, it would have been nice if its public interface had not changed :-). Even if the old interface is now considered faulty in some situations.
Please note that I (personally) am not necessarily suggesting to revert the change. I just wish it had never happened :-) We'll likely patch our local codes to do the test Erik suggested in Paramcheck.
Frank's proposed solution of not syncing for the default values for all the evolution methods would seem the conservative choice to me. Then only the (minority?) of users who use Cowling approximation with Carpet and do not use eg Exact's pseudo-evolution have to change their parameter files (unless they already use "static" anyway).
Yours, Roland
On Wed, Aug 17, 2011 at 11:30 PM, Roland Haas roland.haas@physics.gatech.edu wrote:
Precisely because ADMBase is used by everyone, it would have been nice if its public interface had not changed :-). Even if the old interface is now considered faulty in some situations.
It didn't change... You were performing unnecessary work all along because both ADMBase and your code evolved the ADMBase variables. Since ADMBase came first, your code overwrote this, so no one was hurt; and since ADMBase didn't perform any expensive operations, you didn't notice.
We are discussing here whether to keep a known bug in ADMBase's default parameter settings to help people save time improving performance of private codes that don't follow the ADMBase standard. ADMBase is a really basic, really important thorn for many people, and it needs to be correct and not surprise first-time users. Changing its interface requires much thinking, including about users who are not vocal on these mailing lists.
There's nothing wrong with discussing some of the ADMBase and friends' design decisions; in particular, CoordGauge and StaticConformal come to my mind. But given the user base, this requires careful planning, prominent announcements to the community, and we will have to help people update their codes. We will probably end up supporting both versions for some time. Given that the next ET release focuses on infrastructure improvements, we may want to leave this for another round -- that would then be a "cleanup" release where we focus on clarity, tutorials, debuggability, and improving performance by making sure all the "little things" are in order.
-erik
On 18 Aug 2011, at 15:44, Erik Schnetter wrote:
On Wed, Aug 17, 2011 at 11:30 PM, Roland Haas roland.haas@physics.gatech.edu wrote:
Precisely because ADMBase is used by everyone, it would have been nice if its public interface had not changed :-). Even if the old interface is now considered faulty in some situations.
It didn't change... You were performing unnecessary work all along because both ADMBase and your code evolved the ADMBase variables. Since ADMBase came first, your code overwrote this, so no one was hurt; and since ADMBase didn't perform any expensive operations, you didn't notice.
I don't think ADMBase did any actual "work", it was notionally responsible for evolving its variables, but in practice it did nothing, didn't it? Nothing that the evolution code did was actually being repeated by ADMBase.
We are discussing here whether to keep a known bug in ADMBase's default parameter settings to help people save time improving performance of private codes that don't follow the ADMBase standard.
We are wanting to keep the current behaviour because peoples' private codes assumed things about the way that ADMBase worked for simplicity which were not in accordance with the ADMBase documentation. For reference, the information we have about ADMBase is in http://cactuscode.org/documentation/thorns/CactusEinstein-ADMBase.pdf and https://docs.einsteintoolkit.org/et-docs/Einstein_Toolkit_standards (the former should be considered authoritative).
ADMBase is a really basic, really important thorn for many people, and it needs to be correct and not surprise first-time users. Changing its interface requires much thinking, including about users who are not vocal on these mailing lists.
I'm not sure I fully understand the change that was made. For the case in question, when evolution_method = "static", ADMBase now synchronises all the ADMBase variables on every iteration. From the comment in the schedule.ccl file, it sounds like this synchronisation only needs to happen in certain situations, such as when a new refined grid appears. Wouldn't it be better, even in the case of the Cowling approximation, for these synchronisations to be performed only in those cases, and not on every iteration? This would have the side effect that existing, working, tested, non-conforming codes don't have such a performance hit.
On Thu, Aug 18, 2011 at 10:52 AM, Ian Hinder ian.hinder@aei.mpg.de wrote:
On 18 Aug 2011, at 15:44, Erik Schnetter wrote:
On Wed, Aug 17, 2011 at 11:30 PM, Roland Haas roland.haas@physics.gatech.edu wrote:
Precisely because ADMBase is used by everyone, it would have been nice if its public interface had not changed :-). Even if the old interface is now considered faulty in some situations.
It didn't change... You were performing unnecessary work all along because both ADMBase and your code evolved the ADMBase variables. Since ADMBase came first, your code overwrote this, so no one was hurt; and since ADMBase didn't perform any expensive operations, you didn't notice.
I don't think ADMBase did any actual "work", it was notionally responsible for evolving its variables, but in practice it did nothing, didn't it? Nothing that the evolution code did was actually being repeated by ADMBase.
ADMBase copies the previous timelevels of its variables to the current timelevel. This is (if MoL is used) repeated by MoL. This is a very fast operation, so it won't cause performance problems.
We are discussing here whether to keep a known bug in ADMBase's default parameter settings to help people save time improving performance of private codes that don't follow the ADMBase standard.
We are wanting to keep the current behaviour because peoples' private codes assumed things about the way that ADMBase worked for simplicity which were not in accordance with the ADMBase documentation. For reference, the information we have about ADMBase is in http://cactuscode.org/documentation/thorns/CactusEinstein-ADMBase.pdf and https://docs.einsteintoolkit.org/et-docs/Einstein_Toolkit_standards (the former should be considered authoritative).
ADMBase is a really basic, really important thorn for many people, and it needs to be correct and not surprise first-time users. Changing its interface requires much thinking, including about users who are not vocal on these mailing lists.
I'm not sure I fully understand the change that was made. For the case in question, when evolution_method = "static", ADMBase now synchronises all the ADMBase variables on every iteration. From the comment in the schedule.ccl file, it sounds like this synchronisation only needs to happen in certain situations, such as when a new refined grid appears. Wouldn't it be better, even in the case of the Cowling approximation, for these synchronisations to be performed only in those cases, and not on every iteration? This would have the side effect that existing, working, tested, non-conforming codes don't have such a performance hit.
If this is possible -- yes, definitely. Maybe scheduling in postregrid and postregridinitial would do this? However, the boundary conditions also need to be applied in postrestrict and postrestrictinitial, if the ADMBase variables are restricted (which may or may not make sense). And if the boundary conditions (including synchronisation) are ever applied, they should also be applied in postinitial, so that they are consistent right from the beginning (and don't change after the first regridding).
-erik
Hello Ian, Erik, all,
ADMBase copies the previous timelevels of its variables to the current timelevel. This is (if MoL is used) repeated by MoL. This is a very fast operation, so it won't cause performance problems.
Before you wonder when that happened: It does so since r48 from January 2010 (it seems). Before nothing was copied. We are about 1 year 7 months too late to complain about *that* change :-)
Yours, Roland
users@lists.einsteintoolkit.org
-
Erik Schnetter -
Frank Loeffler -
Ian Hinder -
Roland Haas