search

SNL-WaterPower / WecOptTool-MATLAB

WEC Design Optimization Toolbox
GNU General Public License v3.0
12 stars 9 forks source link

Reduce to structured example with library functions. #161

Closed H0R5E closed 3 years ago

H0R5E commented 4 years ago

So, I have modified the basic.m RM3 example to a barebones pair of function calls, supported by some library functions to help out. I haven't updated the optimisation example yet, as I have to finish for the day.

Let me know your thoughts.

Closes #142 Closes #123

TODO:

H0R5E commented 4 years ago

OK, the optimisation example for RM3 is working in this structure now as well. I've removed all the base types classes and the types function (SeaState has been moved to the top level), the Blueprint and Device classes and recreated all of the functionality directly in the script. It's a little less neat but certainly more transparent.

H0R5E commented 4 years ago

OK, so I have restructured the toolbox into a single namespace (WecOptTool) which is more in keeping with this being a support library. As part of that I have broken out the WecOptLib.utils package into a package called system and a package called math, as this was mostly what the functions in there were doing. I have also moved the tests out of the toolbox and into the root directory. To do this I consolidated the tests.data package into the SeaState class. I also added the regularWave function to the SeaState class.

The main task left (beyond finishing the waveBot example) is to update the inline and web docs for this new structure as they are now completely out of whack. I will start working on this next.

ryancoe commented 3 years ago

@H0R5E - I made some decent progress on this today, but really not as much as I might have hoped. Most of what I did shouldn't have broad consequences, but here's two things that may:

  1. I updated /examples/WaveBot/simulateDevice.m to use MATLAB's argument validation framework and use name-value pairs to handle things like Zmax, Fmax, etc.

https://github.com/SNL-WaterPower/WecOptTool/blob/1705a256ec938ac62d274b11685a747d18f9a8e5/examples/%2BWaveBot/simulateDevice.m#L24

  1. I didn't like how we had files/functions with the same names for each of the examples. Not sure what the best solution is, but I went ahead and used Package Folder naming (+WaveBot) to alleviate this (1705a256ec938ac62d274b11685a747d18f9a8e5). Feel free to revert this if you're not a fan.
H0R5E commented 3 years ago

@ryancoe, I have no issues with 1., seems sensible given we have sacrifised a lot of error checking that was provided by the framework.

Regarding 2., I don't think it's the right approach. I understand you want to make them unique, but these examples should never be added to the path, so the functions will only be accessible locally and therefore they don't need to have unique names (and even if the folders are added to the path, they will be scoped locally first, so only the calling functions would need to be unique). Secondly, having repeated names is useful as it provides a standardised approach to setting up the problem which we can document (i.e. "create two functions with this purpose...", etc), and it's easy for users to start from an existing example and then modify. Thirdly, it adds unnecessary complication, and removing complication is one of the drivers behind the approach in this PR. In my opinion, it's outside of the user domain to start adding packages - they can if they want, but I don't think it's something we should make it look like they need to do. Yeah, so, in conclusion, I think we should reverse this one.

ryancoe commented 3 years ago

@H0R5E - I thought more about point 2 over the weekend. Given how MATLAB searches the working directory at the top of the search path, I do think there's good reason to distinguish the names. We would not be forcing users to apply this approach in their own models, but just making the examples less prone to errors. For example, if you are in examples/RM3 running that basic.m, and then you open and run examples/WaveBot/WaveBot_caseA.m without changing directories, it will try to call the RM3 functions. Its true that the +dir naming is meant for packages, but it is really just a search path tool (unless I'm missing something).

H0R5E commented 3 years ago

You've gotta love MATLAB! The alternative option would be to put them into a private folder which would only allow them to be called by a non private function in the same folder. That's maybe a less complex approach to packages while being more robust.

On Mon, Aug 3, 2020, 15:09 Ryan Coe notifications@github.com wrote:

@H0R5E https://github.com/H0R5E - I thought more about point 2 over the weekend. Given how MATLAB searches the working directory at the top of the search path, I do think there's good reason to distinguish the names. We would not be forcing users to apply this approach in their own models, but just making the examples less prone to errors. For example, if you are in examples/RM3 running that basic.m, and then you open and run examples/WaveBot/WaveBot_caseA.m without changing directories, it will try to call the RM3 functions. Its true that the +dir naming is meant for packages, but it is really just a search path tool (unless I'm missing something).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SNL-WaterPower/WecOptTool/pull/161#issuecomment-668043567, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABG2KF7YSPU72AXWHDW5FS3R63AIFANCNFSM4PJANN5Q .

H0R5E commented 3 years ago

For instance if someone copied and pasted the wavebot example without changing the names you could get the same problem.

On Mon, Aug 3, 2020, 17:06 Mathew Topper dataonlygreater@gmail.com wrote:

You've gotta love MATLAB! The alternative option would be to put them into a private folder which would only allow them to be called by a non private function in the same folder. That's maybe a less complex approach to packages while being more robust.

On Mon, Aug 3, 2020, 15:09 Ryan Coe notifications@github.com wrote:

@H0R5E https://github.com/H0R5E - I thought more about point 2 over the weekend. Given how MATLAB searches the working directory at the top of the search path, I do think there's good reason to distinguish the names. We would not be forcing users to apply this approach in their own models, but just making the examples less prone to errors. For example, if you are in examples/RM3 running that basic.m, and then you open and run examples/WaveBot/WaveBot_caseA.m without changing directories, it will try to call the RM3 functions. Its true that the +dir naming is meant for packages, but it is really just a search path tool (unless I'm missing something).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SNL-WaterPower/WecOptTool/pull/161#issuecomment-668043567, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABG2KF7YSPU72AXWHDW5FS3R63AIFANCNFSM4PJANN5Q .

ryancoe commented 3 years ago

On further thought, let's go with the way you originally had it. I still think some other solution would be good, but I'd rather get this wrapped up first.

H0R5E commented 3 years ago

@ryancoe I tried to recreate the issue you described _"if you are in examples/RM3 running that basic.m, and then you open and run examples/WaveBot/WaveBotcaseA.m without changing directories". I put disp("WAVEBOT"); into \examples\WaveBot\designDevice.m and \examples\WaveBot\simulateDevice.m then (from the root directory) I did:

cd examples\RM3
run("..\WaveBot\WaveBot_caseA.m")

and the (abridged) output that was returned was:

WAVEBOT
Simulation 1 of 3
WAVEBOT
Simulation 2 of 3
WAVEBOT
Simulation 3 of 3
WAVEBOT

          AvgPow     |MaxPow|    PowTHD_dBc     MaxPos     MaxVel     MaxPTO
          _______    ________    __________    ________    _______    ______

    CC    -146.54     996.52        NaN         0.22899    0.45799    3711.8
    P     -43.098     86.196        NaN        0.051564    0.10313    835.82
    PS    -123.81     626.55        NaN         0.16071    0.31584      2002

So, it looks like the folder where the script is being run from is taking precedence. The important part is that these folders shouldn't be placed on the path.

EDIT: In fact, you get the same behaviour if they are added to the path. I would be interested to know if this isn't what happens for you, because then there is a platform issue.

H0R5E commented 3 years ago

Test results:

test_results.pdf

ryancoe commented 3 years ago

I was running tests and figured I'd let you know that this is passing on macOS w/ 2020b Update 4 test_results.pdf