Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement SplineStructure superclass of SplineObject #122

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

TheBB
Copy link
Member

@TheBB TheBB commented Aug 21, 2020

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
Copy link
Member Author

TheBB commented Aug 21, 2020

Everything is functionally identical except there are two new methods:

  • .structure() discards the control point array and returns a pure SplineStructure object.
  • .with_controlpoints() returns a new SplineObject with a different control point array. Works both for SplineObjects and SplineStructures.

@TheBB
Copy link
Member Author

TheBB commented Aug 21, 2020

The CI failure is unrelated.

@TheBB TheBB marked this pull request as draft August 21, 2020 13:43
@VikingScientist
Copy link
Member

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

  • SplineObject -> GeometricObject
  • SplineModel -> MultiPatchModel
  • SplineStructure -> FunctionSpace

@TheBB TheBB reopened this May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants