Bug in caching of graphical surfaces

[wphooper]

There is a bug with the caching of graphical surfaces when the underlying surface is changed. This is illustrated by the following code:

from flatsurf import *
s = translation_surfaces.square_torus()
s = s.triangulate().copy(relabel=True, mutable=True)
show(s.plot())
s.triangle_flip(0, 0, in_place=True)
show(s.plot())
s.graphical_surface(cached=False).plot()

The second image shown should be the changed surface, but you don't get the correct plot until you explicitly disable the cache.

The issue is that the cached graphical surface is stored in SimilaritySurface which isn't notified when the underlying Surface mutates. There are various ways to fix this. Perhaps the easiest is to refuse to produce a GraphicalSurface when the underlying surface is mutable. This would solve the problem of GraphicalSurfaces stored by the user as illustrated below:

from flatsurf import *
s = translation_surfaces.square_torus().triangulate().copy(relabel=True, mutable=True)
gs = s.graphical_surface()
s.triangle_flip(0, 0, in_place=True)
show(gs.plot())
s.graphical_surface(cached=False).plot()

If we don't consider the second code example a bug, then the first example could be solved by storing the cached graphical surface in the underlying surface's cache.

[saraedum]

This will likely be fixed by my changes to make SimilaritySurface a parent as this deprecates mutating of SimilaritySurface objects.

[wphooper]

I'm a bit worried depreciating mutation of these objects will break a lot of code I've written. I assume it will still be possible to mutate the underlying Surface. Is that right? If so, I can presumably modify my code to work around this.

[saraedum]

It should not break much existing code™. When you mutate a similarity surface in the future, you get a deprecation warning (telling you to mutate a copy of the underlying Surface instead) and we will wipe some UniqueRepresentation caches for SimilaritySurface and its subclasses. After such a wipe, pickling won't be compatible with SageMath's test suite anymore, i.e., loads(dumps(similarity_surface)) is similarity_surface might fail and loads(dumps(surface_point)) == surface_point might fail. Also, comparing similarity_surfaces with == and their points with == might produce wrong results when surfaces from before and after the wipe are involved.

[saraedum]

@wphooper do you have some typical code? I would like to add it as a test to see that the warnings are reasonable and that the code still works.

[wphooper]

There are a bunch of methods I've written that have an in_place=False keyword. Here are some that come up in a grep search

• In the HalfDilationSurface class:
  • The apply_matrix method.
  • l_infinity_delaunay_triangulation (This I assume was written by Vincent)
• In SimilaritySurface:
  • set_vertex_zero (This should probably be moved to Surface)
  • relabel
  • triangle_flip
  • join_polygons
  • reposition_polygons
  • triangulate
  • delaunay_triangulation
• In TranslationSurface:
  • standardize_polygons
  • canonicalize

There is example test code in most of these methods. Unfortunately when I was looking I didn't usually include a test covering the case in_place=True. On the other hand, the way the functions are typically written, if the in_place=False, you first make a mutable copy and then run the in_place routine on the copy. So, I tend to think things will break even in the provided example code.

In defense of this mess, the way I think about it is the Surface just provides the basic data, and the encompassing SimilaritySurface (or extension) provides methods which are geometrically natural for the surface given its type. But, often the types of things you want to do to a surface are informed by this geometrical information, so it makes sense for these objects to be mutable. Otherwise, every time you make a change in a geometric algorithm that uses data coming from TranslationSurface (for instance), you'll need to re-encapsulate the Surface in the TranslationSurface class to do computations before figuring out what to do next. (This is assuming that TranslationSurface provides methods which are valuable to your algorithm.)

[saraedum]

There are a bunch of methods I've written that have an in_place=False keyword. Here are some that come up in a grep search

I am aware of these and I transform them so they do not produce any warnings when in_place == False.

There is example test code in most of these methods. Unfortunately when I was looking I didn't usually include a test covering the case in_place=True.

Ok. Then I will use the existing implementations as test cases. If they don't break (but only print warnings and break pickling,) then my changes should not break most other code out there.

In defense of this mess, the way I think about it is the Surface just provides the basic data, and the encompassing SimilaritySurface (or extension) provides methods which are geometrically natural for the surface given its type.

Yes, that's also my understanding. In SageMath lingo, the subclasses of SimilaritySurface could probably even be considered categories.

But, often the types of things you want to do to a surface are informed by this geometrical information, so it makes sense for these objects to be mutable. Otherwise, every time you make a change in a geometric algorithm that uses data coming from TranslationSurface (for instance), you'll need to re-encapsulate the Surface in the TranslationSurface class to do computations before figuring out what to do next. (This is assuming that TranslationSurface provides methods which are valuable to your algorithm.)

In the examples you give above I have not found such a case (yet.) In any case, it appears to me that recreating that TranslationSurface is extremely cheap as most (all?) of the subclasses of SimilaritySurface are just stateless wrappers of Surface. The only state comes from cached computations but these are typically forbidden on mutable surfaces anyway.

[saraedum]

graphical_surface is not cached anymore since #220.