ReScience
[Re] Curve-Fitting with Piecewise Parametric Cubics
Original article: [Re] Curve-Fitting with Piecewise Parametric Cubics
PDF URL: https://github.com/danperazzo/plass_reproduction/blob/main/ReScience/article.pdf Metadata URL: https://github.com/danperazzo/plass_reproduction/blob/main/ReScience/metadata.yaml Code URL: https://github.com/danperazzo/plass_reproduction
Scientific domain: Computer Graphics Programming language: Python Suggested editor: Bruno Levy
Thanks for the submission. I'll handle the editing process and assign reviewers soon.
@HinTak Could you be interested in reviewing this replication of "Curve-Fitting with Piecewise Parametric Cubics"? (the review process is rather light since it is a replication). If you have questions, I'll be happy to answer.
@BrunoLevy Thanks you so much. The review for ReScience C is rather light because it is a replication (and the oroginal article is quite old)
@rougier @BrunoLevy Any news about this submission? I'm one of the authors. Thanks.
@rossant Could make a review of this replication (review is ratherl light)?
Thanks for some interesting reading. I am not sure how the review process works yet, so I'll just comment as I read the paper then the code as I go along.
My first comment (editorial) is "...meet at each knot have the same the tangent directions (not vectors!)." at top of page 2 should read "... have the same tangent[ial] directions..." . "...the same the..." sounds odd.
I'll find the 83 paper and look at the code tomorrow.
Two interesting questions I like to ask would be: does it work well at recovering the original control points if the source contains cubic splines? Slight complication as the original itself might not be mathematically optimal (e.g. the convention of having control points at both the horizontal/vertical outer edge, for hinting purposes). The other question is, how well does it work against glyphs having a lot of straight edges and corners - e.g. a glyph from the Adobe Sans CJK fonts. That is a useful application but also controversial - the typical CJK typefaces have a minimum of ~3000 glyphs (and x10 is not uncommon), and font vendors don't take kindly to theft of copyright via curve fitting of bitmaps.
@HinTak Thanks for your initial comments.
"...the same the..." is a typo; thanks for spotting this.
The method does not attempt to recover the original control points if the source contains cubic splines. It does not model design intentions and knows nothing about typography. On the other hand, it does recover about the same number of curves, which is somewhat surprising.
Figure 2d shows that the method recovers glyphs with straight edges and corners.
@HinTak Thansk, the sooner, the better @BrunoLevy Any update on your review?
I have given the code itself a try. Looks pretty cool. What I do I have to do in terms of review?
I would suggest one small change / improvement: I was checking what temp files it generates (with git status --ignored) and somewhat surprised to see outputs/S_bezier.txt. Then I checked that this is hardcoded in draw_single_image.py. I think I am confused by the S_ part specifically - when I am passing "C.txt" etc as input. I would suggest either derive the output name from the input e.g "basename-saved-bezier.txt", or, if that's too much work, just change it to a more generic "saved-bezier.txt" rather than "S_bezier.txt".
You can find information for review at: https://rescience.github.io/edit/. The main criterion is whether authors manage to replicate the code (do they compare original vs new results) and then you can advise on the manuscript and code (as you already). Once satisfied with feedback/correction from authors, you can recommend for publication. This review can be only a few lines if you think the paper is already on good shape.
I think I am generally quite happy for the paper to go forward, with just correcting that little typo/editorial change. As for the software, besides the little change I would suggest for the default output file name, I have a few more suggestions:
Going back to the paper, I see there is a regularization parameter Tau I can change. So I tried looking for it with -h. I think I see three issues there:
- There is a default value for epsilon, and it seems to be 2. It would be nice to state that like so
--max-distance epsilon Epsilon value for the RDP algorithm (default 2) --disable_rdpdoes not take an argument?- I think the regularization parameter Tau is what you called in
-h:--tolerance Tolerance Tolerance for the dynamic programming algorithm. If that's the case, you probably should fix the help message to look a bit like--regularization-parameter Tau Tolerance for the dynamic programming algorithm.
Or maybe I am a bit confused by what the parameters are called in -h vs what's in the paper?
Lastly, more on the software - I see that you have non-English comments (and also non-ascii comments, for that reason). Those two are somewhat frown upon - on reaching the widest audience (English still being the main medium of global communication), and also potential problems of the code passing through processing that expect plain (ascii, 7-bit) text.
I see it would probably be good to state the default for window_size and num_steps too. Sorry for nit-picking a bit : if there is a default value which is not obvious ( e.g. --scale 1, --initial-value 0), it would be nice to see it documented in the -h message.
I think that's all I have to say. Thanks for showing me an interesting piece of work.
@HinTak Thank you very much for your comments. We'll make the changes in the text and in the code as suggested. We'll post a note here where it's done.
@HinTak We have revised the paper. The revised version is available at https://github.com/danperazzo/plass_reproduction/blob/main/ReScience/article.pdf
Besides the typo you have found, we have fixed a few sentences to make the text clearer.
The original text is at https://github.com/danperazzo/plass_reproduction/blob/main/ReScience/submitted.pdf
Thanks for you comments.
Hello everyone, I just refactored the code to address the concerns raised by @HinTak:
- Updated the descriptions of the argparse
- Changed the name of the
tau_toleranceparameter - Changed the name of the defaul output file to
outputs/saved_bezier.txt, as requested - Regarding
--disable_rdp, transformed it into astore_trueargument for the argparser
@HinTak @BrunoLevy @rougier We have now revised the paper and the code to address your suggestions. We hope that it's all good now. Thank you for your suggestions and for handling our submission.
@HinTak @BrunoLevy @rougier Any further suggestions or recommendations for our submission? Thanks.
I haven't checked - I believe you have addressed the comments / suggestions if you say so.
@lhf I've sent email to @BrunoLevy. Let's wait a few more days and if no news, I'll do the review. Sorry for the delay.