2010/6/29 Jonathan Guyer <gu...@nist.gov> > > > On Jun 29, 2010, at 9:41 AM, Benny Malengier wrote: > > > First, I'd like to mention I'm happy with how fipy is handling the > problem I'm throwing at it. > > I just write here to offer some insight from a user using fipy for the > first time. > > Thank you for sharing your experiences (and for submitting patches!). > > > I have struggled with a bug for a long time because it was unclear that a > Grid2D created from > > from fipy import * > > can be a Grid2D or a UniformGrid2D > > > > I think it would be a better design if Grid2D in meshes.numMesh is > renamed NonUniformGrid2D. > > > > I also find it impossible to write generic code that can work on all > meshes. UniformGrid2D does not call the __init__ method of it's parent > class. I find that a dangerous way of writing inheritance. Eg, for > UniformGrid2D, self.areaProjections attribute is not present, altough it is > an attribute of the parent object (it is set in __init__ of > /meshes/common/mesh.py). > > The UniformGrid?D.areaProjections attribute does not (and will not) exist. > The purpose of the UniformGrid?D classes is that they don't store anything. > The UniformGrid?D._getAreaProjections() method *does* exist, though, and is > what should be called if you need these values. > > We recognize that the modern solution to this is to have a > UniformGrid?.areaProjections @property, but that was not available when this > part of FiPy was written and other tasks have take precedence over > refactoring this. Pervasive introduction of properties is planned, though. > > > but it would be nice if this is not needed, eg by adding the __init__ or > by making in the manual mush clearer that Grid2D is not the nummesh/Grid2D. > > > Renaming fipy.meshes.numMesh.grid?D.Grid?D to > fipy.meshes.numMesh.nonUniformGrid?D.NonUniformGrid?D and then introducing a > common base class for NonUniformGrid?D and UniformGrid?D almost certainly > can and should be done. Likewise, we can certainly clarify the documentation > for the Grid?D() factory functions. The current arrangement is an artifact > of history; Grid?D existed first and was not changed when we introduced > UniformGrid?D for memory efficiency. I can see that the current names can be > confusing. > > We are not likely to add a call to the base class's __init__() as that > would defeat the point of the UniformGrid?D classes (i.e., that they store > virtually nothing and calculate what they need on the fly). > > While invoking base's __init__() is typical, I am not aware that it is a > requirement so long as the subclass implements the entire published > interface of its base classes (and neither areaProjections or > _getAreaProjections() are intended to be part of the published interface). > If you know of any pronouncements from BDFL on the matter, I'd be happy to > see them. > > The problem with not calling __init__ is that people who jump into the code cannot trust the order of calling without always checking.
In my coding, for the case of UniformGrid, I call the __init__ method, but have the init method call functions that can be overridden. In this case, UniformGrid would call init, but would override the sections it does not want. That would allow to have common code still at the most relevant level (like eg self.scale which is a copy of self.scale in the init of common/mesh.py, making rename of a key more work and more error prone). As an example of my way of handling this, I have reworked periodicgrid2d in my patch to allow periodic grids in 2D in all configurations, see http://matforge.org/fipy/attachment/ticket/298/fullperiodic2d_new02.patch The call of _makeperiodic(self), and overwriting that, allows the inheriting classes to call init on the parent module. It is not a big difference, just something I find more clear to read while going over the code. Should I in that patch not have moved self.nonPeriodicCellVertexIDs = Grid2D._getCellVertexIDs(self) to inside the connectFaces function (to make it work on Grid2D or gmsh grids), the code reuse would be nicely illustrated by using the _makeperiodic function. Greetings, Benny > A complete refactoring of the mesh classes is probably warranted, but there > are some conflicting inheritance relationships that so far elude a clearer > arrangement. Proposals welcome. > >
