Clean-up responseClass #491

akeeste · 2021-01-06T22:48:35Z

This PR cleans up the eulXYZ2RotMat and rotMatXYZ2Eul functions appended on to the responseClass. New functions are created in one location that can be called by the classes and Simulink.

Similarly, rotateXYZ and offsetXYZ functions are created so they can be tracked in a single location. The old methods are removed from the classes and simulink, and changed to point to the scripts in the function directory.

nathanmtom · 2021-01-07T23:00:38Z

@akeeste Just a quick clarification, but is the intent here to move more of the functions coded within the the WEC-Sim Library to external MATLAB functions that allow for better version control?

Therefore, you haven't changed any of the code just where it is found?

nathanmtom · 2021-01-07T23:18:17Z

source/functions/orbital_velocity.m

@@ -1,5 +1,5 @@
 function [u,v,w,xa,ya,za] = orbital_velocity(disp,k,omega,waveAmp,theta,t,rampT, beta, h, g)


@akeeste I'm struggling to find where this function is used within WEC-Sim. Although, we need to calculate the flow velocity and acceleration to calculate the Morison Element forces, we do this in the ME block and do not reference a separate function. Trying to determine if this is perhaps an outdated function that could be removed?

I just looked at the orbital velocity calculation in the ME block, it looks like an updated / rewritten version of the stand-alone function. It seems that it's likely outdated and not used

Thanks, that was my initial conclusion after reviewing this file and noticing this is not called in the ME block. If no one else on the WEC-Sim team has a reason to keep this function then suggest removing before pushing to the dev branch.

@nathanmtom I agree

akeeste · 2021-01-07T23:25:38Z

@nathanmtom Yes the functions are moved to a single location so that they are easier to track. They are not changed functionally.

I hadn't really looked at orbital_velocity, just noticed a typo and corrected it. I just searched the entire source directory but can't find anywhere that it is called

nathanmtom · 2021-01-07T23:30:22Z

source/objects/bodyClass.m

-            rotMat(3,3) = ax(3)*ax(3)*(1-cos(t))    + cos(t);
-            xn = x*rotMat;
-        end
+%         function xn = rotateXYZ(obj,x,ax,t)


Currently, these lines are commented out. After this pull request is approved and before pushing to the dev branch will these lines be removed from the bodyClass.m? This comment also goes for offsetXYZ.m file below.

Yes I meant to remove those comments, I'll push that before merging with dev

nathanmtom · 2021-01-07T23:32:01Z

source/objects/constraintClass.m

-            rotMat(3,3) = ax(3)*ax(3)*(1-cos(t))    + cos(t);
-            xn = x*rotMat;
-        end
+%         function xn = rotateXYZ(obj,x,ax,t)


Repeating comment, but suggest removing the commented lines before final push to the dev branch.

nathanmtom · 2021-01-07T23:33:15Z

source/objects/mooringClass.m

-            rotMat(3,3) = ax(3)*ax(3)*(1-cos(t))    + cos(t);
-            xn = x*rotMat;
-        end
+%         function xn = rotateXYZ(obj,x,ax,t)


Repeat comment, about removing commented sections before pushing to dev branch.

nathanmtom · 2021-01-07T23:34:00Z

source/objects/ptoClass.m

-            rotMat(3,3) = ax(3)*ax(3)*(1-cos(t))    + cos(t);
-            xn = x*rotMat;
-        end
+%         function xn = rotateXYZ(obj,x,ax,t)


One more documentation of removing commented lines to clean up code.

nathanmtom · 2021-01-07T23:41:46Z

@akeeste All of the changes .m files look good from my review. The last item I want to review is the library, but since it is more difficult to see where you made changes could you quickly identify which blocks you had to modify?

dav-og · 2021-01-08T00:01:45Z

source/functions/eulXYZ2RotMat.m

@@ -0,0 +1,22 @@
+function rotMat = eulXYZ2RotMat(phi, theta, psi)


This is probably a broader point...but have we decided on a naming convention for functions?

My personal preference would be eulxyz_2_rotmat(), but I think I used eulXYZ2RotMat() originally in keeping with the rest of the code. But IIRC there were mixed conventions and we wanted to resolve that problem? Are we going for camel case function names?

@kmruehl

in fact, the orbital_velocity() function below has an underscore. I would prefer underscores for functions, camel case for variables. Let me know what you think 👍

I agree the naming convention is a bit inconsistent, especially with variables. I think this is a broader cleanup item on the project board.

I think we have waited to address this so far because there were concerns about backwards compatibility if a lot of the class variables are renamed

yeah that's a good point, it will take time to propagate the convention throughout the whole code. But perhaps this PR is a good moment to decide on a convention? (If we haven't already)

Once we decide on coding conventions, we can also include that in the developer documentation section @H0R5E

@dav-og I was going to bring up the convention today, but ran out of time.

How about we leave these camel case for now? The rest of the functions in /source/functions are currently camel case. Though I agree, we should have a different convention for variables, functions, classes. Maybe the team can discuss during the working meeting.

I would love to see some kind of coding guidelines introduced. It would be great to see a Python style line length restriction, as some of the lines are way too long at the moment. Consistent naming style is great too - I'm not sure if you necessarily need to distinguish every type of variable, but consistency helps a lot.

I wouldn't recommend implementing this in a big bang style, however, as you won't be able to properly review such a huge change. This kind of thing is best implemented as other changes are being made, at least until there is very little left to modify.

I think someone should open an issue to discuss it. We might need a doodle poll to vote on the options!

Another thing is you can get a handle on the "accepted" naming conventions from the MATLAB docs. E.g:

Classes, classMethods and Properties

functionDefinitions

variables_whoKnows

There isn't any clarity on how a variable should be named. Personally I prefer UPPERCASE GLOBALS, for instance.

@dav-og and @akeeste we decided on camel case for WEC-Sim a long time ago (many years ago), and I think we should stick with it. Yes, we do have functions/variables that did not follow this naming convention, and we should try to clean them up this year as we we come across them. This is the general cleanup on the project board @akeeste referred do.

akeeste · 2021-01-08T00:20:15Z

@nathanmtom I just pushed a commit to cleanup the commented functions.

The simulink blocks that I removed the functions from are:

nonlinear wave elevation / instantaneous water free surface
hydro body / nonlinear hydrostatic restoring force
hydro body / diffraction force / nonlinear FK force
flex body / nonlinear hydrostatic restoring force
flex body / diffraction force / nonlinear FK force

In each case, the function is changed at the lowest level in the imbedded matlab function of each block.
Let me know if I can clarify further so that you don't have to dig for them

…into cleanup_respClass

dav-og · 2021-01-08T02:58:15Z

The simulink blocks that I removed the functions from are:
* nonlinear wave elevation / instantaneous water free surface

* hydro body / nonlinear hydrostatic restoring force

* hydro body / diffraction force / nonlinear FK force

* flex body / nonlinear hydrostatic restoring force

* flex body / diffraction force / nonlinear FK force
In each case, the function is changed at the lowest level in the imbedded matlab function of each block.
Let me know if I can clarify further so that you don't have to dig for them

I think the quaternion2EulXYZ.m file you've created should be called in Hydrodynamic Body/Structure/Motion Sensor as well?

Also in the description of that function:

%   Q : 1 x 4 float vector
%       List of quaternions

I don't think its a list of quaternions? Just one quaternion

akeeste · 2021-01-13T16:26:11Z

@dav-og I'll update the descriptions for the quarternions. There's not really a change here, as I didn't end up making it an external function.

I can make it an external function, so that git can track a text file. Before we do that across the library though, I'm curious to know if there is a speed difference in how simulink calls functions (interpreted matlab function vs embedded matlab function vs calculated in simulink, etc).

kmruehl · 2021-01-16T01:48:40Z

@akeeste I'm not sure about the speed difference, but I'm inclined to move to do an external function just so that we can make our jobs easier on SCM. I'm willing to take the hit on speed, assuming it's not a huge difference. If you're curious you could compare the time :)

akeeste · 2021-01-18T15:28:18Z

@dav-og @kmruehl
I updated the description of the quaternion function and changed it from an 'interpreted matlab function'.
According to this description, the interpreted function is slightly slower but more importantly it does not support code generation for rapid-accelerator mode.

My solution was to keep an embedded matlab function (untracked), but only use it point to an external matlab function that can be tracked, i.e.:
function E = fcn(Q)
% Call quaternion matlab function.
% quaternion2EulXYZ() saved as separate text file for Github tracking purposes.
E = quaternion2EulXYZ(Q);
end

I'm curious to see Hunter's solution in the PR that he will post. Hopefully that will give us another solution, or fix the tracking problem entirely as is.

dav-og · 2021-01-20T23:57:02Z

hey @akeeste

@dav-og @kmruehl
I updated the description of the quaternion function and changed it from an 'interpreted matlab function'.

the updated description states:

Euler Angle Type II or Tait-Bryan Angle for sequence 321 or zyx (Nautical or Cardan angles).

I think its 'Euler/Tait-Bryan angles; x-y-z extrinsic convention'

akeeste · 2021-01-21T00:02:33Z

@dav-og
Yes you're right, I believe it uses the xyz convention. I need to merge dev into this PR, I'll change the description when I do that and then merge.

akeeste added 4 commits January 5, 2021 15:18

clean up rotMatXYZ2Eul and eulXYZ2RotMat functions

dfabe49

clean up and make consistent ofsetXYZ and rotateXYZ functions

c011893

clean up and make consistent the quaternion to euler angle functions

745e597

revert quaternion2EulXYZ to matlab function in Simulink

d4cecf8

kmruehl requested a review from nathanmtom January 6, 2021 23:32

dav-og self-requested a review January 6, 2021 23:32

akeeste changed the base branch from master to dev January 6, 2021 23:33

nathanmtom reviewed Jan 7, 2021

View reviewed changes

dav-og added 3 commits January 7, 2021 16:53

extra detail in eulXYZ2RotMat() description

4cdf276

updated rotMatXYZ2Eul() description

965c323

updated eulXYZ2RotMat() definition

941f149

dav-og reviewed Jan 8, 2021

View reviewed changes

clean up commented functions

d913654

Merge branch 'cleanup_respClass' of https://github.com/akeeste/WEC-Sim …

ebb2149

…into cleanup_respClass

kmruehl added the Response Class Response Class (responseClass.m) label Jan 16, 2021

update quaternion function description and calling method

8b33b1c

remove orbital_velocity function

0add69d

akeeste added 2 commits January 21, 2021 10:11

pull dev and update quaternion function description

fb4c0f7

pull changes into new dev paraview functions

b8436e9

akeeste merged commit 976b5cb into WEC-Sim:dev Jan 21, 2021

akeeste deleted the cleanup_respClass branch January 21, 2021 16:39

		@@ -1,5 +1,5 @@
		function [u,v,w,xa,ya,za] = orbital_velocity(disp,k,omega,waveAmp,theta,t,rampT, beta, h, g)

		@@ -0,0 +1,22 @@
		function rotMat = eulXYZ2RotMat(phi, theta, psi)

Clean-up responseClass #491

Clean-up responseClass #491

Uh oh!

Conversation

akeeste commented Jan 6, 2021

Uh oh!

nathanmtom commented Jan 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akeeste commented Jan 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanmtom commented Jan 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dav-og Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akeeste commented Jan 8, 2021

Uh oh!

dav-og commented Jan 8, 2021

Uh oh!

akeeste commented Jan 13, 2021

Uh oh!

kmruehl commented Jan 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akeeste commented Jan 18, 2021

Uh oh!

dav-og commented Jan 20, 2021

Uh oh!

akeeste commented Jan 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dav-og Jan 8, 2021 •

edited

Loading

kmruehl commented Jan 16, 2021 •

edited

Loading