From: Vijay S. M. <vi...@gm...> - 2010-01-13 05:34:00
|
Hi all, I just recently noticed the inclusion of "virtual" keyword on 3 methods in EquationSystems. I have derived my physics from EquationSystems and have used it that way for a while now. I had to make few of the routines virtual in order to accomplish that but since I did not see any of that in the existing design, I assumed that EquationSystems was not supposed to be extendable as is. But the new inclusions have got me all confused. I do notice that the new changes might be modifications in progress but since my code depends on this object intricately, I am interested in the reasons for this move. If it is not too much of a detailed explanation, I would very much appreciate it if you can explain this design decision. Thanks a lot ! Vijay |
From: John P. <pet...@cf...> - 2010-01-13 15:19:22
|
On Tue, Jan 12, 2010 at 11:33 PM, Vijay S. Mahadevan <vi...@gm...> wrote: > Hi all, > > I just recently noticed the inclusion of "virtual" keyword on 3 > methods in EquationSystems. I have derived my physics from > EquationSystems and have used it that way for a while now. I had to > make few of the routines virtual in order to accomplish that but since > I did not see any of that in the existing design, I assumed that > EquationSystems was not supposed to be extendable as is. But the new > inclusions have got me all confused. > > I do notice that the new changes might be modifications in progress > but since my code depends on this object intricately, I am interested > in the reasons for this move. If it is not too much of a detailed > explanation, I would very much appreciate it if you can explain this > design decision. > > Thanks a lot ! Hmm... 'svn annotate' says the solve() function and the destructor have been virtual since version 1811, that version dates back to 2007-01-22. The other two virtual functions are more recent, from 3517, added by Roy for adjoint and sensitivity solve stuff. I'd imagine none of these really affects what you are doing with EquationSystems, does it? -- John |
From: Roy S. <roy...@ic...> - 2010-01-13 15:53:43
|
On Tue, 12 Jan 2010, Vijay S. Mahadevan wrote: > I had to make few of the routines virtual in order to accomplish > that but since I did not see any of that in the existing design, I > assumed that EquationSystems was not supposed to be extendable as > is. But the new inclusions have got me all confused. Your confusion is due to the fact that you're assuming our design is better than it is. ;-) The explanation is simple: In principle, we've thought that EquationSystems should be usable as a base class. In particular, I think that overriding solve() would be one of the more elegant ways to implement certain operator splitting and loose coupling algorithms. In practice, none of the primary developers have derived from EquationSystems, so features necessary to do it well may be missing because they've never been tested in practice. Which routines did you make virtual? There's nothing in EquationSystems that gets called from an inner loop, so there should be no objections to making more of its methods virtual in the SVN head. --- Roy |
From: Derek G. <fri...@gm...> - 2010-01-13 23:31:22
|
On Jan 13, 2010, at 8:53 AM, Roy Stogner wrote: > In practice, none of the primary developers have derived from > EquationSystems, so features necessary to do it well may be missing > because they've never been tested in practice. BTW - I am thinking of doing this... so this discussion was interesting to me. Simultaneously... I don't have a strong opinion on any of the topics... Basically this was a useless email. Derek |
From: Vijay S. M. <vi...@gm...> - 2010-01-13 18:08:39
|
Sorry about the delay in reply. Roy, > Your confusion is due to the fact that you're assuming our design is > better than it is. ;-) haha. libMesh is one of the more decently structured libraries I've seen and you guys have done a great job in making sure the design adheres to many of the standard patterns/principles. It is not a perfect design but any change that improves it does help everyone. Yes. I can see that in solve, all the systems are treated in a completely decoupled fashion, in a block-jacobi sense. But without the ability to derive from EquationSystems explicitly, there is not much use for this routine. It is understandable that all the primary developers have not derived from this object since it is supposed to be self contained. In my version of EquationSystems, I have the following methods made virtual which I think is the minimal set needed to derive from it. void clear () void init () void reinit () System & add_system (const std::string &system_type, const std::string &name) bool compare (const EquationSystems &other_es, const Real threshold, const bool verbose) const std::string get_info () const Also, I derived EquationSystems from ReferenceCountedObject. Do let me know what you think. John, I know solve was virtual for a while now but only noticed the other virtual methods recently. I do not have adjoint sensitivity analysis capability in my code yet and definitely do not use the new routines yet. And that could've been the reason why I was oblivious to the change. On Wed, Jan 13, 2010 at 9:53 AM, Roy Stogner <roy...@ic...> wrote: > > On Tue, 12 Jan 2010, Vijay S. Mahadevan wrote: > >> I had to make few of the routines virtual in order to accomplish >> that but since I did not see any of that in the existing design, I >> assumed that EquationSystems was not supposed to be extendable as >> is. But the new inclusions have got me all confused. > > Your confusion is due to the fact that you're assuming our design is > better than it is. ;-) The explanation is simple: > > In principle, we've thought that EquationSystems should be usable as a > base class. In particular, I think that overriding solve() would be > one of the more elegant ways to implement certain operator splitting > and loose coupling algorithms. > > In practice, none of the primary developers have derived from > EquationSystems, so features necessary to do it well may be missing > because they've never been tested in practice. > > Which routines did you make virtual? There's nothing in > EquationSystems that gets called from an inner loop, so there should > be no objections to making more of its methods virtual in the SVN > head. > --- > Roy > |
From: Roy S. <roy...@ic...> - 2010-01-13 18:22:24
|
On Wed, 13 Jan 2010, Vijay S. Mahadevan wrote: > In my version of EquationSystems, I have the following methods made > virtual which I think is the minimal set needed to derive from it. > > void clear () > void init () > void reinit () > System & add_system (const std::string &system_type, const std::string &name) > bool compare (const EquationSystems &other_es, const Real threshold, > const bool verbose) const > std::string get_info () const > > Also, I derived EquationSystems from ReferenceCountedObject. Do let me > know what you think. Most of that makes perfect sense. I'll add all of it in the SVN head. What do you do in the overridden add_system()? --- Roy |
From: Vijay S. M. <vi...@gm...> - 2010-01-13 20:37:15
|
Roy, the other reason that add_system needs to be virtual is because in EquationSystems.read(), the systems are created while reading the header through the call to add_system(type, name). And so if you have a System that is not recognized by libmesh, this will always throw an error while reading the header. I hope that is a more sensible reason to make this method virtual. Its been a while since I made this change on my end and when I removed the virtual call on this method, my program started crashing and the reason became apparent. Vijay On Wed, Jan 13, 2010 at 12:22 PM, Roy Stogner <roy...@ic...> wrote: > > On Wed, 13 Jan 2010, Vijay S. Mahadevan wrote: > >> In my version of EquationSystems, I have the following methods made >> virtual which I think is the minimal set needed to derive from it. >> >> void clear () >> void init () >> void reinit () >> System & add_system (const std::string &system_type, const >> std::string &name) >> bool compare (const EquationSystems &other_es, const Real threshold, >> const bool verbose) const >> std::string get_info () const >> >> Also, I derived EquationSystems from ReferenceCountedObject. Do let me >> know what you think. > > Most of that makes perfect sense. I'll add all of it in the SVN head. > > What do you do in the overridden add_system()? > --- > Roy > |
From: Roy S. <roy...@ic...> - 2010-01-13 21:29:19
|
On Wed, 13 Jan 2010, Vijay S. Mahadevan wrote: > Roy, the other reason that add_system needs to be virtual is because > in EquationSystems.read(), the systems are created while reading the > header through the call to add_system(type, name). And so if you have > a System that is not recognized by libmesh, this will always throw an > error while reading the header. I hope that is a more sensible reason > to make this method virtual. Its been a while since I made this change > on my end and when I removed the virtual call on this method, my > program started crashing and the reason became apparent. Ah, yes. I forgot about the tests in add_system(). Redefining add_system() to handle every SpecialSnowflakeSystem class is a dangerous way to go, though. Then we write "SpecialSnowflakeSystem" to output files, try to run those output files later through a more general utility that doesn't know the class exists, and "ERROR: Unknown system type:" ensues. Notice that DiffSystem and FEMSystem don't override ImplicitSystem::system_type(). The same goes for my own personal CahnHilliardSystem, NavierStokesSystem, SpecialSnowflakeSystem, etc. classes. That way, my saved output all gets written as "ImplicitSystem", which can be read from generic libMesh utilities without crashing. When they're read as restart files, however, and I actually really *do* want to create a SpecialSnowflakeSystem, that's still easy enough: the code that's reading them is SpecialSnowflakeSystem-aware, so it calls es.add_system<SpecialSnowflakeSystem>("Special") itself. Then when es.read() encounters a system named "Special" in the data file, it fills in the data for the existing system of that name. --- Roy |
From: Vijay S. M. <vi...@gm...> - 2010-01-13 21:51:22
|
I understand that every "SpecialSnowflakeSystem" does not need special attention and in case I gave you that impression, sorry about that. But the fact is that I do not use EquationSystems directly but instead only rely on the derived member, say CustomEquationSystem, which is aware of the necessary calls to create, manipulate and delete SpecialSnowflakeSystem. Hence, CustomEquationSystem should override add_system(type, name) and will perform an action only if the type is SpecialSnowflakeSystem. Else, it will just forward the call to EquationSystems::add_system(type, name). This IMO is cleaner and allows good flexibility without breaking existing behavior. This argument is valid for someone who is interested in deriving from EquationSystems and want to create, use their own SpecialSnowflakeSystem implementation. Otherwise, what you suggest is true and I would not want to touch add_system for every new "RandomSystem" created by a user. > When they're read as restart files, however, and I actually really > *do* want to create a SpecialSnowflakeSystem, that's still easy > enough: the code that's reading them is SpecialSnowflakeSystem-aware, > so it calls es.add_system<SpecialSnowflakeSystem>("Special") itself. > Then when es.read() encounters a system named "Special" in the data > file, it fills in the data for the existing system of that name. Btw, this will not work as is. Because, the read() method calls add_system(type, name) immaterial of whether such a system exists currently or not. May be a call to has_system() before this would actually implement the behavior you specify. Vijay On Wed, Jan 13, 2010 at 3:29 PM, Roy Stogner <roy...@ic...> wrote: > > On Wed, 13 Jan 2010, Vijay S. Mahadevan wrote: > >> Roy, the other reason that add_system needs to be virtual is because >> in EquationSystems.read(), the systems are created while reading the >> header through the call to add_system(type, name). And so if you have >> a System that is not recognized by libmesh, this will always throw an >> error while reading the header. I hope that is a more sensible reason >> to make this method virtual. Its been a while since I made this change >> on my end and when I removed the virtual call on this method, my >> program started crashing and the reason became apparent. > > Ah, yes. I forgot about the tests in add_system(). > > Redefining add_system() to handle every SpecialSnowflakeSystem class > is a dangerous way to go, though. Then we write > "SpecialSnowflakeSystem" to output files, try to run those output > files later through a more general utility that doesn't know the class > exists, and "ERROR: Unknown system type:" ensues. > > Notice that DiffSystem and FEMSystem don't override > ImplicitSystem::system_type(). The same goes for my own personal > CahnHilliardSystem, NavierStokesSystem, SpecialSnowflakeSystem, etc. > classes. That way, my saved output all gets written as > "ImplicitSystem", which can be read from generic libMesh utilities > without crashing. > > When they're read as restart files, however, and I actually really > *do* want to create a SpecialSnowflakeSystem, that's still easy > enough: the code that's reading them is SpecialSnowflakeSystem-aware, > so it calls es.add_system<SpecialSnowflakeSystem>("Special") itself. > Then when es.read() encounters a system named "Special" in the data > file, it fills in the data for the existing system of that name. > --- > Roy > |
From: Roy S. <roy...@ic...> - 2010-01-13 22:05:16
|
On Wed, 13 Jan 2010, Vijay S. Mahadevan wrote: > But the fact is that I do not use EquationSystems directly but instead > only rely on the derived member, say CustomEquationSystem, which is > aware of the necessary calls to create, manipulate and delete > SpecialSnowflakeSystem. Hence, CustomEquationSystem should override > add_system(type, name) and will perform an action only if the type is > SpecialSnowflakeSystem. Else, it will just forward the call to > EquationSystems::add_system(type, name). Right. So your code will work, because it's using CustomEquationSystem. But when I try to open your file in my meshdiff, meshplot, meshimage, etc. utilities, they'll crash, because they're using EquationSystems. Likewise, if I'd used your method, none of the files I'd written out would be openable in any code you've written. It's okay if you don't have such generic tools or don't need that level of interoperability, but it's not cleaner or more flexible. >> When they're read as restart files, however, and I actually really >> *do* want to create a SpecialSnowflakeSystem, that's still easy >> enough: the code that's reading them is SpecialSnowflakeSystem-aware, >> so it calls es.add_system<SpecialSnowflakeSystem>("Special") itself. >> Then when es.read() encounters a system named "Special" in the data >> file, it fills in the data for the existing system of that name. > > Btw, this will not work as is. When theory and practice give differing answers, it's time to reexamine the theory... > Because, the read() method calls add_system(type, name) immaterial > of whether such a system exists currently or not. May be a call to > has_system() before this would actually implement the behavior you > specify. Look at the add_system() implementation. Like with add_vector(), you can add an already-existant name and you'll just get a reference to the already-added object. --- Roy |
From: Roy S. <roy...@ic...> - 2010-01-13 22:20:57
|
On Wed, 13 Jan 2010, Roy Stogner wrote: > When theory and practice give differing answers, it's time to > reexamine the theory... Hmmm... rereading this sentence in context, it (and my choice of "SpecialSnowflakeSystem") sounds derogatory, whereas I was aiming more for "humorous". Something got lost in the translation from my head to ASCII. I apologize if I offended. (I don't mind occasionally sounding like an offensive jackass on the internet, mind you, but I'd prefer if it was always intentional) --- Roy |
From: Vijay S. M. <vi...@gm...> - 2010-01-13 22:19:47
|
> Right. So your code will work, because it's using > CustomEquationSystem. But when I try to open your file in my > meshdiff, meshplot, meshimage, etc. utilities, they'll crash, because > they're using EquationSystems. Likewise, if I'd used your method, > none of the files I'd written out would be openable in any code you've > written. True. You will not be able to read my output as is but my library still derives from libMesh and so anything created in libMesh can be read by my code. But vice-versa will not work since libMesh does not know about my SpecialSnowFlakeSystem or my derived EquationSystems object. This is fine since my application is a little specific to the physics I'm solving and is not a very general library yet. May be in the future ... > Look at the add_system() implementation. Like with add_vector(), you > can add an already-existant name and you'll just get a reference to > the already-added object. I understand that. Again, I was talking about SpecialSnowFlakeSystem which will fail all the "ifs" and hence throw an error at the end, even if it has been added by the SnowFlake aware CustomEquationSystem a-priori. SpecialSnowFlakeSystem is the reason why I've been arguing to make add_system(type, name) virtual. If no one else finds this useful, I can make this as my specific change in my working copy. Vijay On Wed, Jan 13, 2010 at 4:05 PM, Roy Stogner <roy...@ic...> wrote: > > > On Wed, 13 Jan 2010, Vijay S. Mahadevan wrote: > >> But the fact is that I do not use EquationSystems directly but instead >> only rely on the derived member, say CustomEquationSystem, which is >> aware of the necessary calls to create, manipulate and delete >> SpecialSnowflakeSystem. Hence, CustomEquationSystem should override >> add_system(type, name) and will perform an action only if the type is >> SpecialSnowflakeSystem. Else, it will just forward the call to >> EquationSystems::add_system(type, name). > > Right. So your code will work, because it's using > CustomEquationSystem. But when I try to open your file in my > meshdiff, meshplot, meshimage, etc. utilities, they'll crash, because > they're using EquationSystems. Likewise, if I'd used your method, > none of the files I'd written out would be openable in any code you've > written. > > It's okay if you don't have such generic tools or don't need that > level of interoperability, but it's not cleaner or more flexible. > >>> When they're read as restart files, however, and I actually really >>> *do* want to create a SpecialSnowflakeSystem, that's still easy >>> enough: the code that's reading them is SpecialSnowflakeSystem-aware, >>> so it calls es.add_system<SpecialSnowflakeSystem>("Special") itself. >>> Then when es.read() encounters a system named "Special" in the data >>> file, it fills in the data for the existing system of that name. >> >> Btw, this will not work as is. > > When theory and practice give differing answers, it's time to > reexamine the theory... > >> Because, the read() method calls add_system(type, name) immaterial >> of whether such a system exists currently or not. May be a call to >> has_system() before this would actually implement the behavior you >> specify. > > Look at the add_system() implementation. Like with add_vector(), you > can add an already-existant name and you'll just get a reference to > the already-added object. > --- > Roy > |
From: Roy S. <roy...@ic...> - 2010-01-13 22:31:15
|
On Wed, 13 Jan 2010, Vijay S. Mahadevan wrote: > I understand that. Again, I was talking about SpecialSnowFlakeSystem > which will fail all the "ifs" and hence throw an error at the end, Not if you don't override its parent's system_type() method. Then the string that gets written out (and so eventually read in) will match a libMesh parent class, the add_system<Parent>("name") call will be made. Because a system with "name" already exists the add_system() call will just do a get_system(), and because Parent is a parent class of SpecialSnowflake the dynamic_cast in get_system will still be happy. I'm not just guessing or deducing this behavior; this is how my code works, and it doesn't throw errors in EquationSystems::read(). > even if it has been added by the SnowFlake aware CustomEquationSystem > a-priori. SpecialSnowFlakeSystem is the reason why I've been arguing > to make add_system(type, name) virtual. If no one else finds this > useful, I can make this as my specific change in my working copy. We'll leave it in the SVN head. It's a trivial overhead, and even if there's a better solution for your problem, it might be useful for other reasons we haven't thought of yet. --- Roy |
From: Vijay S. M. <vi...@gm...> - 2010-01-14 02:43:52
|
Roy, no offense taken. Its just a name.. If there are valid criticisms, I always welcome it and if not, I do not bother with it much. > I'm not just guessing or deducing this behavior; this is how my code > works, and it doesn't throw errors in EquationSystems::read(). I know. This was the behavior I noticed for a long time. When I made my changes to add my new System class, I originally did not change the system_type. But later when I did, my restart files did not work any more. And I read the code for EquationSystems, System and XDR before making the changes I did. This was not a whimsical move because it was the easiest. This was what made perfect sense back then. Now that its been more than 8 months since I made this change, I cannot remember all the reasons for it... > We'll leave it in the SVN head. It's a trivial overhead, and even if > there's a better solution for your problem, it might be useful for > other reasons we haven't thought of yet. Thanks. I have bunch of custom changes in my working copy and am just trying to eliminate them in order to make my merges easy. If any of this does not make sense, do not worry about it. You could always say NO because these are just suggestions so that at some point in the future, I can probably converge my version with the trunk while making appropriate changes in my code. Vijay On Wed, Jan 13, 2010 at 4:31 PM, Roy Stogner <roy...@ic...> wrote: > > On Wed, 13 Jan 2010, Vijay S. Mahadevan wrote: > >> I understand that. Again, I was talking about SpecialSnowFlakeSystem >> which will fail all the "ifs" and hence throw an error at the end, > > Not if you don't override its parent's system_type() method. Then > the string that gets written out (and so eventually read in) will > match a libMesh parent class, the add_system<Parent>("name") call will be > made. Because a system with "name" already exists the add_system() > call will just do a get_system(), and because Parent is a parent class > of SpecialSnowflake the dynamic_cast in get_system will still be happy. > > I'm not just guessing or deducing this behavior; this is how my code > works, and it doesn't throw errors in EquationSystems::read(). > >> even if it has been added by the SnowFlake aware CustomEquationSystem >> a-priori. SpecialSnowFlakeSystem is the reason why I've been arguing >> to make add_system(type, name) virtual. If no one else finds this >> useful, I can make this as my specific change in my working copy. > > We'll leave it in the SVN head. It's a trivial overhead, and even if > there's a better solution for your problem, it might be useful for > other reasons we haven't thought of yet. > --- > Roy > |