Property validation for WEC-Sim objects #904
Conversation
source/objects/bodyClass.m
Outdated
| 'threshold', 1) % (`structure`) Defines the passive yaw mplementation. ``option`` (`integer`) Flag for passive yaw calculation, Options: 0 (off), 1 (on). Default = ``0``. ``threshold`` (`float`) Yaw position threshold (in degrees) above which excitation coefficients will be interpolated in passive yaw. Default = ``1`` [deg]. | ||
| centerBuoyancy (:,1) double = [] % (`3x1 float vector`) Body center of buoyancy [m]. Defined in the following format [x y z]. For hydrodynamic bodies this is defined in the h5 file while for nonhydrodynamic bodies this is defined by the user. Default = ``[]``. | ||
| centerGravity (:,1) double = [] % (`3x1 float vector`) Body center of gravity [m]. Defined in the following format [x y z]. For hydrodynamic bodies this is defined in the h5 file while for nonhydrodynamic bodies this is defined by the user. Default = ``[]``. | ||
| dof (1,1) double {mustBeNonnegative, mustBeInteger} = 6 % (`integer`) Number of degree of freedoms (DOFs). For hydrodynamic bodies this is given in the h5 file. If not defined in the h5 file, Default = ``6``. |
There was a problem hiding this comment.
@jtgrasb This is interesting as I would have expected that if you wrote i = 1 that MATLAB would have assigned it as an integer but it is a float and if you use the isinteger function within MATLAB it will return a 0 logical. Therefore, in the % comment should we change this to float or double to be consistent with what the property validation is actually checking for? What is written the checkinputs is looking for a (1,1) double not a (1,1) integer.
There was a problem hiding this comment.
@nathanmtom I had not checked the fact that it isn't actually considered an integer type by MATLAB - that is interesting. In this case (for body.dof), the mustBeInteger function checks the value to ensure its an integer (even though the type is double). Personally, I think leaving it as integer here provides more info to the user even though its not entirely accurate for variable type. Similarly, I left the term 'float' in the comments as I felt like that was a more common term vs. double is more obscure (although more technically correct). Let me know what you think -- I am open to suggestions here.
source/objects/simulationClass.m
Outdated
| cicEndTime (1,:) double {mustBeScalarOrEmpty} = 60 % (`float`) Convolution integral time. Default = ``60`` s | ||
| domainSize (1,1) double {mustBePositive} = 200 % (`float`) Size of free surface and seabed. This variable is only used for visualization. Default = ``200`` m | ||
| explorer (1,:) char = 'on' % (`char array`) SimMechanics Explorer 'on' or 'off'. Default = ``'on'`` | ||
| dt (1,1) double {mustBePositive} = 0.1 % (`float`) Simulation time step. Default = ``0.1`` s |
There was a problem hiding this comment.
@jtgrasb I wanted to verify order of operations in checking the inputs. For dt it appears that MATLAB first checks the (1,1) double before checking {mustBePositive}. Therefore, if I input simu.dt = [-0.1, -0.1] the error will mention the vector needs to be a scale but will not additionally mention the variable needs to be positive. No change is required, but thought this would be relevant to clarify.
There was a problem hiding this comment.
@nathanmtom That's a good point. Do you think this should be mentioned in documentation somewhere?
source/objects/simulationClass.m
Outdated
| saveText (1,1) double = 0 % (`integer`) Flag to save results as ASCII files, Options: 0 (off), 1 (on). Default = ``0`` | ||
| saveWorkspace (1,1) double = 1 % (`integer`) Flag to save .mat file for each run, Options: 0 (off), 1 (on). Default = ``1`` | ||
| simMechanicsFile (1,:) char = 'NOT DEFINED' % (`char array`) Simulink/SimMechanics model file. Default = ``'NOT DEFINED'`` | ||
| solver (1,:) char = 'ode4' % (`char array`) PDE solver used by the Simulink/SimMechanics simulation. Any continuous solver in Simulink possible. Recommended to use 'ode4, 'ode45' for WEC-Sim. Default = ``'ode4'`` |
There was a problem hiding this comment.
@jtgrasb I have been spot checking the "checks", but when I write simu.solver = 1 the checkinputs does not seem to catch this and the error that pops up basically says this is not a valid solver. Therefore, in my opinion it does not seem to be catching this case.
There was a problem hiding this comment.
@nathanmtom It seems the char specification tries to convert the number to a char array, which leads to just an empty char. I am adding a check in simu.checkInputs to ensure the solver is one of Simulink's solvers.
| largeXYDisplacement (1,1) struct = struct(... % | ||
| 'option', 0) % | ||
| linearDamping double = zeros(6) % (`6x6 float matrix`) Defines linear damping coefficient matrix. Default = ``zeros(6)``. | ||
| mass (1,:) = [] % (`float`) Translational inertia or mass [kg]. Defined by the user or specify 'equilibrium' to set the mass equal to the fluid density times displaced volume. Default = ``[]``. |
There was a problem hiding this comment.
@jtgrasb Another spot check, but if I update the OSWEC example with body(1).mass = [1,1] the simulation runs and errors out at a different point rather than in checking the input file. I realize this is difficult because this variable is either a double or a char array and cover both is challenging. Since the error was caught later this probably is a minor comment, but raises a separate question if we could convert equilibrium to say -1, and so we can keep mass as a (1,1) double and in the code.
There was a problem hiding this comment.
@nathanmtom Yes, covering both is challenging, but changing equilibrium to -1 could be a potential solution. I'll also look into adding a check in the checkInputs.
source/objects/ptoClass.m
Outdated
| 'rotationMatrix', []) % (`structure`) Defines the orientation axis of the pto. ``z`` (`3x1 float vector`) defines the direciton of the Z-coordinate of the pto, Default = [``0 0 1``]. ``y`` (`3x1 float vector`) defines the direciton of the Y-coordinate of the pto, Default = [``0 1 0``]. ``x`` (`3x1 float vector`) internally calculated vector defining the direction of the X-coordinate for the pto, Default = ``[]``. ``rotationMatrix`` (`3x3 float matrix`) internally calculated rotation matrix to go from standard coordinate orientation to the pto coordinate orientation, Default = ``[]``. | ||
| pretension = 0 % (`float`) Linear PTO pretension, N or N-m. Default = `0`. | ||
| stiffness = 0 % (`float`) Linear PTO stiffness coefficient. Default = `0`. | ||
| damping (1,:) double {mustBeNonnegative} = 0 % (`float`) Linear PTO damping coefficient. Default = `0`. |
There was a problem hiding this comment.
@jtgrasb I believe the size of the damping variable should be (1,1), I just tired with a (1,2) and it passed this session but crashed in Simulink.
There was a problem hiding this comment.
@nathanmtom The damping, stiffness, waves.height, and waves.stiffness are all part of our MCR workflow which allows users to enter vectors for these variables. If I restrict the size to (1,1), it causes errors with this MCR workflow.
There was a problem hiding this comment.
@jtgrasb Thanks for the clarification, that makes sense.
|
@jtgrasb Thank you for starting this pull request and after my initial review this PR should help users identify issues with the wecSimInputFile.m much quicker. From my preliminary review, I do see many of the variable checks working as expected, while I did work with trying to break the OSWEC Example wecSimInputFile.m and noticed some of the checks you have implemented did not work as desired and have left comments on those I caught. There are a lot of cases to check and we do not have one WEC-Sim model that you can easily check all of the cases quickly, but perhaps we ensure that all of the checks work with the OSWEC and RM3 examples first. |
|
@nathanmtom Thanks for all the comments -- they have been helpful to identify areas to improve the PR. There are definitely a lot of cases to check -- I tried to do my best to check where I expected difficulties, but clearly missed a few. I also ran the WEC-Sim Applications tests, which helped me catch some more errors and are all passing now. |
Add solver check
|
I was doing a bit more testing of some of the validations and came across some unintended issues where the validation was converting things such as character arrays to doubles instead of restricting the input types. This thread gives the details on a similar issue and why it occurs: https://www.mathworks.com/matlabcentral/answers/504887-checking-datatype-instead-typecast-at-property-or-argument-validation |
|
@nathanmtom @kmruehl I've made some updates to this PR:
Let me know what you guys think about these updates/potential issues. |
|
@jtgrasb Thank you for all of the updates related to this PR, understandable that there are a lot of potential combinations to check for and generalize. After reviewing all of the latest updates, I have no major comments but responded below to a few of your questions.
Since this PR is not affecting the dynamic simulation portion of WEC-Sim, I am in favor of making the I'll wait for @kmruehl to provide any comments if they would prefer to wait before merging. |
|
@nathanmtom Thanks for the suggestions! I changed "char array" back to "string" for the documentation and also added string as a valid input type for the mass (accepts both "equilibrium" and 'equilibrium'). |
|
Just wanted to update. I am getting this same documentation error when I try to build locally. Not sure of the exact cause. |
|
The documentation errors have been fixed and it is working locally. It seems to only "fail" here due to a warning. |
|
@jtgrasb thank you. I'll go ahead and merge this. I think we should remove the fail on warnings for docs. |
This PR is meant to add more checks to the user inputs to WEC-Sim. Property validation is added to WEC-Sim objects to restrict input variables by size, type, and other validation functions.
Outside of the property validation itself, some other notes/inclusions in this PR:
stringproperty label in API tochar arraybecause strings and character arrays are different in MATLAB and we use character arrays as defaults (although the input also accepts strings and converts them to character arrays)