A3.7 & A3.8 from The Nurbs Book

[Nodli]

Hi,
I implemented both algorithms in SurfaceEvaluator2. Both deriv_ctrlpts and derivatives_single have tests in test_BSpline_Surface.py. derivatives_single is compared with its equivalent in SurfaceEvaluator1.

I added tests for deriv_ctrlpts for Curve in test_BSpline_Curve2D.py and test_BSpline_Curve3D.py.

Everything was tested with pytest for Python 2 and Python 3.
I ran prospector for Python 2 on what I added and left only the warnings related to super() and the number of variables in the functions. I tried the Python 3 version in case there are differences but could not make it work.

I have two suggestions related to these functions :

• I think default values for r1, r2, s1, s2 in deriv_ctrlpts should be the number of control points so that all control points of the derivated curve / surface are returned when the function is called without these arguments
• I would like to include a method in Surface and Curve to return the output of deriv_ctrlpts

Let me know your thoughts on these, I can implement them if you agree.

[codecov]

Codecov Report

Merging #33 into master will increase coverage by 0.66%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   76.78%   77.44%   +0.66%     
==========================================
  Files          12       12              
  Lines        3088     3174      +86     
  Branches      668      686      +18     
==========================================
+ Hits         2371     2458      +87     
+ Misses        578      576       -2     
- Partials      139      140       +1

Impacted Files	Coverage Δ
geomdl/evaluators.py	99.1% <100%> (+0.16%)	⬆️
geomdl/BSpline.py	67.56% <100%> (+0.72%)	⬆️
geomdl/Abstract.py	59.41% <100%> (+0.51%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59dc8c4...27b0209. Read the comment docs.

[orbingol]

I think you have deleted CONTRIBUTING.md by mistake :) That would be a good idea to undo it using git revert {commit_hash}. These things could happen sometimes, no worries. One suggestion here: You don't have to pull the changes on the master all the time. When you create a PR, it will show (I guess in the box at the bottom of the PR page) whether your branch is good to merge or not. If it is not, then you may do the necessary changes as instructed on the box.

Regarding to your suggestions:

I would like to include a method in Surface and Curve to return the output of deriv_ctrlpts

I guess this is also possible. However, this kind of change will require a little bit more than you may anticipate, for instance dealing the with the abstract methods. Let me explain what I was thinking while working on the evaluators:

An evaluator should provide the basic evaluation functionality, such as single and multiple knot point and derivative evaluation (you may consider checking out Abstact.Evaluator class). Therefore, a user who wants to implement a completely new evaluator with different algorithms (I believe other spline algorithms could fit here) is asked to implement 4 methods in his/her new evaluator class. On the other hand, if any more advanced algorithms or some custom functionalities are required, then the user may create a second class (e.g. Abstract.CurveEvaluator or Abstract.SurfaceEvaluator classes) and use multiple inheritance to extend his/her evaluator with the new functionality. This is the case for the Knot Insertion algorithms in curves and surfaces.

For the derivatives of the control points case, what I would recommend is first adding the method (e.g. derivatives_ctrlpts) to Abstract.CurveEvaluator or Abstract.SurfaceEvaluator classes (not to the Abstract.Evaluator class). Even though I added staticmethod with the latest commits, it looks like it is not required at all, then the decorator can be removed completely. The new method should be added to the main evaluator implementations, evaluators.CurveEvaluator and evaluators.SurfaceEvaluator and therefore, it will be inherited by evaluators.CurveEvaluator2 and evaluators.SurfaceEvaluator2 by default.

After these implementations, a wrapper method may be added to BSpline.Curve and BSpline.Surface classes and it should be tested with the NURBS module as well.

It looks like a lot of work to do but written explanation takes more time than doing it. If it is too confusing for you, I can take care of this part pretty quickly, since we have all the algorithms right now.

I think default values for r1, r2, s1, s2 in deriv_ctrlpts should be the number of control points so that all control points of the derivated curve / surface are returned when the function is called without these arguments

I think this is acceptable. If it really works in that way with the default values you are providing, then I would not be against the change. Making the things a little bit easier is always accepted.

For the default values, I would suggest putting them inside the wrapper function of the BSpline.Curve and BSpline.Surface classes as the default value of kwargs.get function just to make it a little bit more convenient.

[orbingol]

By the way, I forgot to say thanks in my previous message @Nodl. This is an awesome job, I appreciate your time and efforts. Please let me know about your decision on adding derivatives_ctrlpts.

[Nodli]

Thanks for your comments.
I could definitely do it but i won't have much time until later this week.
I reverted the commit with the CONTRIBUTING file, i don't know how it happened, i thought i had fetched an updated version...

I am doing an internship to implement a nurbs library in C++ and i can't release it as free software, so i am happy to help on this project of yours.

[orbingol]

I could definitely do it but i won't have much time until later this week.

That's perfectly okay for me. Please take your time and let me know if I could do any help.

I reverted the commit with the CONTRIBUTING file, i don't know how it happened, i thought i had fetched an updated version...

Don't worry on these. It happens nearly all the time in my other projects. As I explained before, you don't need to fetch all changes to your project before the merge.

I am doing an internship to implement a nurbs library in C++ and i can't release it as free software, so i am happy to help on this project of yours.

Thank you very much @Nodli. I appreciate your help.

[orbingol]

I guess you have nothing to add to this PR @Nodli. Merging it now. Thanks!

[Nodli]

Actually i was implementing tests for the NURBS module but I think I found a bug in the computation of weights in derivative_ctrlpts for NURBS Curves and Surfaces.
I am currently trying to solve this.

[orbingol]

Anyway, I have already merged it. I will open a new issue for this problem.