SINTEF / Splipy

Spline modelling made easy.
GNU General Public License v3.0
100 stars 18 forks source link

Implement SplineStructure superclass of SplineObject #122

Open TheBB opened 3 years ago

TheBB commented 3 years ago

One of the design choices made in nutils is that it makes sense to separate topology and data as much as possible. It's a point of view that I find I'm increasingly sympathetic to, and I find it sometimes quite annoying that in Splipy I'm forced to always have a control point array for each SplineObject even when what I'm really doing is having a spline 'structure' (that is, a set of bases) and a number of fields defined on that structure (that is, control point arrays).

I'd like to be able to use Splipy in this way. If it won't be the officially sanctioned usage pattern then I'd at least like it to be possible.

As a first step, this PR creates a superclass of SplineObject called SplineStructure, which has everything that a SplineObject has minus a control point array. I moved most of the methods that were possible to define only in terms of bases to the super class. Some other methods for operations that work on both the bases and the control points, I've implemented as a superclass method that can be called from the subclass.

These operations also make sense to move either wholly or partially to the superclass, but I've left them alone for now: lower_periodic, split, make_periodic and make_splines_identical.

In the future I would like to take this one step further: most of these methods should be defined on the SplineStructure superclass, and they should return objects of some new type ControlPointOperation or something like that, which can be applied to the control point arrays. Then the SplineObject class can do stuff like:

def make_periodic(self, *args, **kwargs):
    operation = super().make_periodic(*args, **kwargs)
    self.controlpoints = operation(self.controlpoints)

and in my own code I can do stuff like

operation = structure.make_periodic(*args, **kwargs)
fields = [operation(field) for field in fields]
TheBB commented 3 years ago

Everything is functionally identical except there are two new methods:

TheBB commented 3 years ago

The CI failure is unrelated.

VikingScientist commented 3 years ago

I have no objections for creating a superclass of SplineObject which leaves out controlpoints as long as it is completely backward compatible. It is very important that the natural entry point of the library continuous to be the objects Curve, Surface and Volume and that these objects have good documentation and usabilities. If this vision is kept, then I don't mind the proposed change. And this PR does indeed keep this mind. The only place where this comes close to being challenged is with function signatures such as

def make_periodic(self, *args, **kwargs)

which I've already expressed my concerns about in #88. However this is not something new to this PR.

Maybe reconsider some of the naming schemes as we currently have SplineObject, SplineModel and SplineStructure, which might not be confusing naming schemes. After all, the entire library is Spline-based, so it might be superfluous to have it in the name. I propose