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

Code Review: Documentation #288

Closed
marlitas opened this issue Mar 14, 2023 · 9 comments
Closed

Code Review: Documentation #288

marlitas opened this issue Mar 14, 2023 · 9 comments

Comments

@marlitas
Copy link
Contributor

for #268

Opening this issue to track documentation questions that feel too general for a REVIEW comment, and too small for their own specific issue.

@marlitas marlitas self-assigned this Mar 14, 2023
@marlitas
Copy link
Contributor Author

marlitas commented Mar 14, 2023

TransformedCurve

This is quite a heavyweight in the model. I am by no means fooling myself to insinuate that I tracked all of the math happening in this file, and to a certain extent must trust that @veillette and @pixelzoom have done their research here.

Overall the documentation was very thorough, which allowed me to follow the math in a very superficial manner helping to make the code legible. A couple general comments for this file:

  • I see the word weight everywhere, and though I know it helps indicate how a point should be calculated I've had a hard time actually putting my finger on what is being "weighed" and how. Because of how prevalent this is in the code and documentation, it feels like a more thorough explanation of weight would be helpful.
  • There is some reordering of functions that I think would be helpful for legibility, for example: grouping manipulateCurve and widthManipulatedCurve together, grouping createPedestalAt, createHillAt, createParabolaAt, etc. together. This just makes it easier to navigate the through the file and provides some order.

@marlitas
Copy link
Contributor Author

I ended up not having too many REVIEW comments that fell into this category. The micro cases can be found by searching // REVIEW: in the code.

@marlitas marlitas assigned pixelzoom and veillette and unassigned marlitas Mar 14, 2023
@veillette
Copy link
Contributor

I'll assign this one to myself.

@pixelzoom
Copy link
Contributor

Thanks @veillette. Looks like the remaining REVIEW comments are in your wheelhouse. I moved one to #291, which I'll handle.

veillette added a commit that referenced this issue Mar 15, 2023
Signed-off-by: Martin Veillette <[email protected]>
veillette added a commit that referenced this issue Mar 15, 2023
veillette added a commit that referenced this issue Mar 15, 2023
@veillette
Copy link
Contributor

veillette commented Mar 15, 2023

There is some reordering of functions that I think would be helpful for legibility, for example: grouping manipulateCurve and widthManipulatedCurve together, grouping createPedestalAt, createHillAt, createParabolaAt, etc. together. This just makes it easier to navigate the through the file and provides some order.

I attempted to reorder the methods, but I just realized that we have internal processes (from git-hooks?) that reorder methods behind the scene. Public methods are set above the private methods.
In spite of this constraint, I was able to reorder the methods in a way that can create a more natural grouping.

veillette added a commit that referenced this issue Mar 15, 2023
veillette added a commit that referenced this issue Mar 15, 2023
@pixelzoom
Copy link
Contributor

... we have internal processes (from git-hooks?) that reorder methods behind the scene. P

That's a setting in the WebStorm "Commit Changes" dialog. I keep that turned off, because I don't like WebStorm deciding how my code should be arranged.

screenshot_2431

veillette added a commit that referenced this issue Mar 15, 2023
veillette added a commit that referenced this issue Mar 15, 2023
@veillette
Copy link
Contributor

veillette commented Mar 15, 2023

I added some context for weight functions, their purposes, and the various weight functions that we used in the sim in the header of TransformedCurve

We should note that the TransformedCurve class is the basis of the original curve, and, therefore,
its first, and second derivative will be evaluated. As a result, much effort was spent creating curve manipulations
that yields unusually smooth curves for which their first and second derivatives are themselves smooth.

Most curve manipulations make use of a weight function to "blend" in a curve segment into a previous curve.
A weight function is a mathematical device used when performing an average to give
some elements more "weight" or influence on the result than other elements in the same set.
The result of the application of a weight function is a weighted sum or weighted average.
A variety of weight functions ranging from gaussian kernel, super gaussian, mollifying functions
are used to create curves without cusps and discontinuities.

veillette added a commit that referenced this issue Mar 15, 2023
@veillette
Copy link
Contributor

I have addressed all the REVIEW comments (except the one in GraphNode which is tracked in #291). Unfortunately I have removed the word REVIEW as I worked on them, so there is no easy way for @marlitas to track the work on this issue :(
I'll mention that some of that documentation was vestigial, wrong and/or sometimes lacking. Thanks for pointing those out.

I also ordered the methods and added on comments on 'weight' in TransformedCurve :#288 (comment) .

Assigning back to @marlitas for review.

@marlitas
Copy link
Contributor Author

These commits are very helpful and the reordering seems much cleaner to me! Thanks for clarifying and responding to the REVIEW comments. Closing this issue as completed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants