#18529: Topological manifolds: basics
-------------------------------------+-------------------------------------
       Reporter:  egourgoulhon       |        Owner:  egourgoulhon
           Type:  enhancement        |       Status:  needs_info
       Priority:  major              |    Milestone:  sage-7.0
      Component:  geometry           |   Resolution:
       Keywords:  topological        |    Merged in:
  manifolds                          |    Reviewers:  Travis Scrimshaw
        Authors:  Eric Gourgoulhon,  |  Work issues:
  Travis Scrimshaw                   |       Commit:
Report Upstream:  N/A                |  3cd03a48d847e12745ed8c25b23f19db141c179a
         Branch:                     |     Stopgaps:
  public/manifolds/top_manif_basics  |
   Dependencies:  #18175             |
-------------------------------------+-------------------------------------

Comment (by egourgoulhon):

 Replying to [comment:108 tscrim]:
 >
 > You had many `if self._manifold is self:` statements, which breaks the
 localization principle;

 Well not so many: grep returns only 5 occurences in the whole
 !SageManifolds:
 - in `TopologicalManifold._repr_` for returning "Manifold..." instead of
 "Open subset of manifold..."
 - in `DifferentiableManifold._repr_` for the same reason
 - in `ManifoldSubset.superset`, because a superset can only `self` in case
 of the whole manifold
 - in `ManifoldSubset.intersection`, because the intersection with the
 whole manifold is trivial
 - in `ManifoldSubset.union`, because the union with the whole manifold is
 trivial

 >
 > Additionally, subsets of a manifold are precisely that, subsets. So we
 should encapsulate that in separate classes. Mathematically we identity an
 open subset as a manifold, but by doing so, we loose the information that
 it is really a subset of another manifold. As such, I feel that having
 separate classes actually better reflects the mathematics.

 Actually, at the moment, we are dealing with the open subsets in exactly
 the same way as we are doing with the whole manifold, i.e. we set up
 charts on them, define tensor fields, etc. This can be seen by noticing
 that in Hierarchy-1, the class `OpenTopSubmanifold` has very few methods
 in addition to those inherited from `TopologicalManifold`. Similarly, none
 of the classes `OpenDiffSubmanifold` and `DifferentiableManifold`
 introduces any attribute or method by itself, in addition to those
 inherited from `DifferentiableMixin` (except for their `__init__`).
 Maybe, in a future development, one could have operations that make sense
 only for open subsets or only for the whole manifold. Then having two
 separate classes is clearly the way to go. However,  at the moment I don't
 see any such operation, apart from those listed above: the printout (via
 `_repr_`) and the union/intersection with other subsets.

 >
 > In an ideal world, I would like to have a hierarchy similar to
 Hierachy-2, but with an additional subclass corresponding to each type of
 manifold that is obtained by adding a subset mixin class. Yet, that
 doesn't seem possible with the current core design.
 >
 > While the Hierarchy-1 seems more complicated, it actually will be easier
 to maintain and debug as code is local to the class (i.e., if you are a
 subclass of `A`, then you act in the same way and like `A` irregardless of
 your input).
 >
 > This assessment is based on my experiences and what I learned as good
 OOP principles. However, I have not looked at !SageManifolds beyond this
 ticket at present, whereas you have worked heavily on it. So perhaps we
 can get some more data. How many times before the refactoring did you have
 to do a `self._manifold is self` test?

 Five times, as mentioned above.

 > Also what is your thoughts on maintenance and extendability?

 It seems to me that, in terms of maintenance, Hierarchy-2 is better, being
 simpler; it is also certainly better for somebody new entering into the
 code.
 On the contrary, if one regards extendability, Hierarchy-1 offers the
 possibility to have different operations for manifolds and subsets, as
 mentioned above. This is a good point in favor of it. Even if at the
 moment I don't have in mind any such operation (apart from those listed
 above), I would slightly tend to maintain Hierarchy-1 because of this.

 >
 > Something else might be to get a 3rd party's opinion (Nicolas and/or
 Darij comes to my mind).

 Yes extra points of view would be helpful! In addition to Nicolas and
 Darij, I don't know if other people associated to this ticket (Vincent,
 John, Nils, Jeoren,...) are reading this and have an opinion...

 > I could also sit down and plan out how I would do everything from the
 ground up to see if I can devise a better overall strategy if you think
 there might be some benefit to that at this stage.

 Thanks, but maybe this is not necessary at this stage, especially if we
 conclude that we maintain Hierarchy-1. Since all this has little impact on
 the user interface, maybe we could rediscuss it after having a whole view
 of all the manifold tickets.

--
Ticket URL: <http://trac.sagemath.org/ticket/18529#comment:109>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica, 
and MATLAB

-- 
You received this message because you are subscribed to the Google Groups 
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
To post to this group, send email to [email protected].
Visit this group at https://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.

Reply via email to