Add EllipticArc Module

[garrettbyrd]

This PR implements an EllipticArc module that is used to generate elliptic curves. It behaves similarly to the CircularArc module, except that it requires xRadius, yRadius, (for major and minor axes) and optional rotation parameters.

An example has been included, and the documentation has also been updated. Any usage of "Arc" in the previous documentation has been relabeled as "Circular Arc" for clarity.

Below is a visualization of the region produced from Examples/2D/RotatedEllipseWithSubregions/RotatedEllipseWithSubregions.control:

[DavidAKopriva]

This looks like a useful addition. It supersedes the CircularArc curve. The CircularArc module could be removed if another constructor with only those parameters needed for a circle is added to this class. Discussion is needed on that, since there would be no point in having both in the long run. At first glance, the new code looks clean and tidy.

[fluidnumerics-joe]

The CircularArc module could be removed if another constructor with only those parameters needed for a circle is added to this class. Discussion is needed on that, since there would be no point in having both in the long run. At first glance, the new code looks clean and tidy.

I can work with Garrett to put a generic together for this. Would you want to support both keys in the control file for backwards compatibility?

[DavidAKopriva]

@fluidnumerics-joe Thanks. Yes, both keys should be allowed in the control file, just which constructor is called (and the data type) is all that would be needed to be changed, and we'd have backward compatibility (very important).

[andrewwinters5000]

Thanks for adding this new curve class! I left a few (mainly cosmetic) suggestions.

[garrettbyrd]

Thanks for the corrections. Yes, the rotation is now considered for the exact solution in the test. Benchmark was added to the correct directory as well. (For posterity, I will also note that the flag is -generateTest.) Rotations are also specified for test ellipses to reduce output clutter.

Separate initializers for CIRCULAR_ARC and ELLIPTIC_ARC in SMModel is mostly motivated by clarity of error messages; e.g., "No name found in circular arc curve definition. Using 'circularArc' as default" vs "No name found in elliptic arc curve definition. Using 'ellipticArc' as default", though I suppose this could all be done conditionally within ImportEllipticArcEquationBlock by passing/checking CIRCULAR_ARC_CONTROL_KEY/CIRCULAR_ARC_CONTROL_KEY. Or perhaps some other way I am failing to think of currently.

[DavidAKopriva]

What you have works just fine as it is. I thought about the two "import" routines and don't see any reason to change that. One could be clever with arrays of strings and accessing them according to the type, but that's just not worth doing. So I'm good!

[andrewwinters5000]

[andrewwinters5000]

Just a few final small suggestions. I added an updated figure link such that we can remove the need to add the Allfeatures.png file from the docs folder.

[codecov]

Codecov Report

Attention: Patch coverage is 79.02098% with 30 lines in your changes missing coverage. Please review.

Project coverage is 75.05%. Comparing base (e7ed7d0) to head (f498508).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
Source/Curves/ContinuousCurves/SMEllipticArc.f90	73.07%	14 Missing ⚠️
Source/Project/Model/SMModel.f90	75.43%	14 Missing ⚠️
Source/Testing/CurveTests.f90	93.93%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
+ Coverage   74.88%   75.05%   +0.16%     
==========================================
  Files          68       68              
  Lines       10429    10519      +90     
  Branches        2        2              
==========================================
+ Hits         7810     7895      +85     
- Misses       2619     2624       +5     

Flag	Coverage Δ
unittests	75.05% <79.02%> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[garrettbyrd]

@andrewwinters5000 Thanks for the catches. Makes perfect sense with the AllFeatures image.

Is there an expected way to test for tangentOnEllipticArcAt, printEllipticArcDescription, initDefaultEllipse, and releaseEllipticArc? I don't think the analogous subroutines/function in SMCircularArc were tested for, but I am happy to go beyond a 1-to-1 re-implementation.

[DavidAKopriva]

You would do these in the TestCurves subroutine. Once you set up either the ellipse or the circle you can call the tangent routine and compare the result to the exact. I'm not sure that the initDefaultEllipse serves any purpose, TBH, and I think it can be eliminated, but if you keep it you can call it to init an ellipse and then immediately destruct it. Note that normally the initDefaultXXX would call the "designated initializer", which is "the initializer with the most arguments" i.e. `initWithParametersNameAndID_SMEllipticArc' with a set of default parameters. As for the print routine, I put that in for debugging purposes long ago... it has no useful purpose beyond that so those routines have not been tested. But to test one would open a STATUS='SCRATCH' file and write to that unit and then read it in and see if what is there is correct. But like I said, none of those have been tested for any of the classes. This code was written before testing, and the testing framework was written post code, so tests have been added after the fact as we go along.

[andrewwinters5000]

Thanks @garrettbyrd ! Everything looks good from my side of things. The testing you have added looks good and exercises most of the relevant code.

[garrettbyrd]

All sounds good.

I went ahead and eliminated initDefaultEllipse. I started implementing tests for the non-covered subroutines, and went ahead with completing an implementation for testing printLineDescription and printEllipticArcDescription. Even this is probably a bit beyond the scope of this PR, but it does work to improve coverage. Further tests should reserved for a separate issue or PR, IMO. (I could even move the "Added printDescription" commit to a new branch if that is preferred.)

All of this to say, as for the EllipticArc module, I am content as long as all checks pass.

[DavidAKopriva]

Looks like it passes the tests. I need to look at the documentation to make sure that's up to date (I think Andrew did that) and if so, then I am good to go. I'll be back...

[DavidAKopriva]

Ah, I think you misunderstood the print test. The print argument takes the unit to which the print will be written. So you shouldn't have changed the printDescription routines, but rather open the scratch unit in the test routine and pass that unit number to printDescription.

[andrewwinters5000]

Everything look good to me with respec to the features and testing. Just a point of clarification. For existing control files, as long as one replaces the keyword "Arc" with "Circular Arc" then the appropriate constructor will be called, right? I am looking toward updating the Julia front end to add this new functionality while maintaining backwards compatibility.

[DavidAKopriva]

The control key for a circle hasn't changed. There are two now:
CHARACTER(LEN=STRING_CONSTANT_LENGTH), PARAMETER :: CIRCULAR_ARC_CONTROL_KEY = "CIRCULAR_ARC"
CHARACTER(LEN=STRING_CONSTANT_LENGTH), PARAMETER :: ELLIPTIC_ARC_CONTROL_KEY = "ELLIPTIC_ARC"

Then you can either use the old block as before (backward compatibility) for the circle, or the new one for the ellipse. I have updated the comments in the header of the control file reader with all the keywords.

[DavidAKopriva]

I moved the opening of the scratch files into the test procedure and out of printDescription and updated the comments in SMModel for the new control block for the elliptic curve. The tests run with sufficient coverage now.

[garrettbyrd]

Excellent. Thanks for the test cleanup.