Alpha Testing

[H0R5E]

A list of things to do prior and during alpha testing:

• close Move NEMOH Files to ./~nemoh_runs #64
• close Code Architecture #5
• close Choose WecOptTool license #60
• update readme to add install from releases (in addition to from source) in Add Architecture and Contribution Sections to README #68
• build a release candidate
• gather feedback
• respond to feedback

[ryancoe]

I'd really like to get this to alpha testers by COB today. Please either wrap up the outstanding items or consider them unnecessary (e.g., I'm ok with people cloning the repo).

[H0R5E]

Yeah, I agree. I'm going to merge #68 now and then I will put together a zip file of the code and docs and put it somewhere. The alpha testers can then work from this zip.

[ssolson]

Ryan could you review my comments in the ghPages #63 review while Mat builds the release?

[H0R5E]

Here you go:

wecopttool_v01_rc1.zip

[ssolson]

I have approved the #63 we should update the docs to reflect the changes. I am running the test suite now. Documentation works.

[ssolson]

I had 2 tests fail. I am looking into the call to strsplit() in the tests which run example.m and dependencyCheck.m which I ran independent runTests.m no problem.

test_results.pdf

[ssolson]

This seems to work if I define what the string is first. Not sure what the issue is here. Any suggestions?

K>> strsplit(s.path, '\')
Error using strsplit
No value was given for '\'. Name-value pair arguments require a name followed by a value.

Error in strsplit (line 99)
    p.parse(varargin{:});
 
K>> str = s.path;
K>> strsplit(str, '\')

ans =

  1×8 cell array

  Columns 1 through 7

    {'C:'}    {'Users'}    {'sterl_000'}    {'Desktop'}    {'alphaWOT'}    {'wecoopttool'}    {'toolbox'}

  Column 8

    {'+WecOptLib'}

[ssolson]

Ahh! Its finding both +WecOptLibs. We should flag this as an issue but presumably, users will not have multiple WecOptTool Installations?

K>> s.path

ans =

    'C:\Users\sterl_000\Desktop\alphaWOT\wecoopttool\toolbox\+WecOptLib'


ans =

    'C:\Users\sterl_000\Documents\WecOptTool\toolbox\+WecOptLib'

[H0R5E]

I think this is what being temperamental again. I have a way of getting this path more robustly now, so I'll update it now.

[H0R5E]

OK, that update is in #73. I'll assume it's OK and put together a new RC.

[H0R5E]

RC2!

wecopttool_v01_rc2.zip

[ryancoe]

@kmruehl @zmorrell-sand @dforbush2 - You've all been asked to perform alpha testing reviews of WecOptTool. We realize this is short notice and a short timeline, so we'll be happy to with whatever you're able to provide by COB Friday. Here's some instructions/notes:

To get started, download the release candidate from this thread. This .zip file contains the source code (i.e., the git master) and documentation.

Answering some questions you've had so far:

• Who is the intended audience for this tool? - WEC developers and researchers.
• What are its intended use cases? - The current v0 release is a simple proof of concept, just using the RM3. There is no guidance or documentation at this stage on how to study other devices (this is coming later).
• Do you want feedback on the code itself, documentation, example, or all of the above? - all of the above.
• How should I respond? My suggestion would be respond via a comment on this thread.

Thanks for your help.

[H0R5E]

• Release Candidate 3: documentation updates:
  • wecopttool_v01_rc3.zip

[zmorrell-sand]

I will try setting up and running this on both Windows and Linux to make sure that it is compatible across platforms. I will let you know if I have any questions or find anything weird

[zmorrell-sand]

One minor thing I noticed that may be a good idea to address, the example.m file is dependent on has "use-parallel" option set to True, but this requires the Parallel Computing Toolbox if I am not mistaken. I am unsure of whether MATLAB will default to the non-parallel case if the user does not have the toolbox. We specify in the README and html documentation that the Parallel Optimization Toolbox is optional. I looked into it a little bit, and it seems that license('test', 'Distrib_Computing_Toolbox') can test if someone has access to the toolbox, so perhaps we should place the options inside of an if statement depending on this command?

[zmorrell-sand]

Everything seems to run well on Windows. I made sure to follow the installation instructions directly and ignore any prior knowledge I had about the program. Installation went smoothly and worked out of the box. I made sure to run all the test cases and example scripts, and it all worked well.

Now onto linux.

[kmruehl]

Okay, I'm going to provide my feedback in a few comments. This is what I have thus far.

Index

• Add a more detailed description about what the WecOptTool is and what it's used for. Why do people want to use this tool? What can it do for them? Why should they bother installing/using this toolbox? A user may think it looks like a lot of work to install, and they don't know why they would want to use it.
• Contents/User Guide headers are redundant

Installation

• "Stable or development versions are available." Where is development, in a dev branch? On a fork? Will development be on master, so the "stable" version is the tagged release? How do I contribute do its development?
• I recommend using the .. Note syntax for notes
• Specify version of the dependency, will the MATLAB Optimization Toolbox from 2015b work? I recommend specifying the version you've run as a minimum requirement
• The first step to remove an existing installation is confusing. I recommend making that a note. First step should be download the code via a clone or download.
• For WEC-Sim we have a 'wecsimstartup.m' file that adds the path, see documentation here, you may consider having a startup file in the distribution with a relative path to add the path for the toolbox. You could also check for dependencies. Path issues are one the biggest issues we see, so the easier you can make it on users, the better.
• Should be addpath(genpath('C:\Users\kmruehl\Documents\GitHub\WecOptTool\wecopttool_v01_rc3\wecopttool')), otherwise subfolders are not added to directory.
• The savepath command gave me a weird warning. May consider having users create a 'startup.m' file in their MATLAB path that gets executed every time MATLAB is opened instead of saving the path
• update documentation to specify correct NEMOH bin file directory, it's C:\Users\kmruehl\Documents\GitHub\Nemoh\Release, not Nemoh\bin
• Until I added the genpath, I got this error, Undefined variable "WecOptLib" or class "WecOptLib.utils.getUserPath". when trying to install nemoh. I recommend having an output of the 'installNemoh' function that tells users the install was successful.
• I like the dependencyCheck functionality, but I think it should be added to the end of the NEMOH install function and run automatically.
• I like the runTests functionality, and the command window output it nice, but the documentation should also tell me what a successful 'runtests' looks like.
  Looks like I got a successful run
  Totals: 27 Passed, 0 Failed, 0 Incomplete.

Copyright/License

• Why was GNU open-source license selected?
  This will likely be limiting to developers who want to customize the toolbox for their application.

• Where is the Copyright assertion in the LICENSE file?
  This should go through Sandia legal and have an NTESS assertion.

  <program> Copyright (C) <year> <name of author> This program comes with ABSOLUTELY NO WARRANTY; for details type 'show w'. This is free software, and you are welcome to redistribute it under certain conditions; type 'show c' for details.

  <one line to give the program's name and a brief idea of what it does.> Copyright (C) <year> <name of author>

• Copyright assertion also needs to be added to documentation:
  /wecopttool_v01_rc3/html_docs/user/license.html

• Copyright assertion in source code should not be to "Sandia National Labs". This is what the assertion should look like:
  Copyright 2014 National Technology & Engineering Solutions of Sandia, LLC (NTESS). Under the terms of Contract DE-NA0003525 with NTESS, the U.S. Government retains certain rights in this software.

[zmorrell-sand]

As Kelley mentioned, addpath(genpath('/path/to/wecopttool/')) needs to be used on Linux, or installNemoh will not run properly. Other than that, everything runs well out of the box on Linux as well

[dforbush2]

I was able to install and run on windows, Matlab 2019a. However, my run of example had "Solver Stopped Prematurely" warnings for fmincon.

Code is mostly clear. Its like Matlab wearing a python costume.

WEC-Sim can conflict with WAFO if they are both on the path at the same time, and since you might share some of the user base, I personally like having to manually add WAFO to my path here rather than a startup file, but I could see the benefits either way.

I think for a version 0.1 release you guys look good!

It would be great to see the example documentation fleshed out a little more...at least labeling the output figure (I think its power for each of the 8 query sea-states?).

Thinking longer term, it would be great to see a more populated theory/workflow section to get a better idea of whats under the hood and what user I/Os are available. I'm a little concerned about how extendable this meshing approach will be to non-axisymmetric geometries, as Nemoh leaves some to be desired on this front. If you intend to actively support this code I worry you'll have the same issues we do with WEC-Sim where half of our posted issues are actually Nemoh issues.

[H0R5E]

Thanks everyone for your feedback. My fault about the loss of genpath. Casual change because it wasn't needed in Windows. I'll try and address all your notes over the weekend. Thanks again. T

[kmruehl]

Okay, and here's my second round of comments

Example

• Update link about RM3 to this page: https://tethys-engineering.pnnl.gov/signature-projects/rm3-wave-point-absorber. The Sandia RMP page is being taken down.
• "geometric design variables, a^2" but figure states "r1, r2, d1, d2"
• label the "gray evaluation block" in the figure
• label the inputs and outputs in this figure
• I think there needs to be more of a theatrical explanation of what this toolbox does. The figure is helpful, but it does not provide enough description of the underlying theory behind this approach.
  • What are the inherent assumptions?
  • What are its applications and limitations? It seems to be dependent on linear potential flow theory (e.g. NEMOH), so I assume this means I shouldn't apply this tool for extreme events.
  • When would I want to use this? Once I already have a general shape and I want to tune its general dimensions?
  • I assume I cannot use this to change my geometry completely, right? I can't use it to compare a point absorber to a OSWC, etc. The underlying theory/assumptions/limitations should be explicitly stated.
  • What are the outputs of this tool? The figure doesn't show what I'm optimizing for, cost, power, peak/avg loads, power/weight etc.?
  • What are the inputs to this tool?
  • Am I assuming that the float is a cylinder and that the damping plat is a cylinder? Do they have to have a flat top/bottom? What about the central column, what is its dimension, can I change it?
  • How is the PTO system modeled? Is damping specified, if so where/how, etc?
• Why not use MHKiT instead of WAFO? This is also developed through WPTO funding and we can modify it as need be to support this program. It would be nice to see WPTO funded projects leverage other WPTO funded projects.
• Define Design Variables section needs more clarification. The lower bounds and upper bounds are mentioned without really being introduced. I think it's fairly intuitive, but I recommend making this as clear as possible. Consider adding a Variable definition section to your documentation.

Running the Example

• Section 2.6. Run the study and view results, how dod I execute the example? Do I run the example.m file? it doesn't run if I type WecOptTool.run(study, options); into the command line. This is confusing. It appears that the code is executed by running the example.m file but this is not stated in the example documentation. It seems like this section is actually just explaining the different lines of the example.m file. That's useful, but needs to be stated.
• There needs to be an explanation of what this case is, what its doing, what the inputs are, and what the outputs are.
• I ran the example and got the following results, but do not know what this means or how to use this information.
  Elapsed time is 95.649494 seconds. Optimal solution is: r1: 5 [m] r2: 7.5 [m] d1: 1.125 [m] d2: 42 [m] Optimal function value is: 2202419.3209 [W] Generating plot...
• The plot shows up as Figure 29, even though I have no other figures open.
• My MATLAB Workspace has a lot of variables in it. Is there an output object? If so, which one is it? Consider clearing all internal toolbox variables, and only keeping the inputs/ouputs in the workspace upon completed run
• I recommend focusing your efforts on this section, this is really the meat of the documentation right now. Explain what this example is, how to use it, what the I/Os are, and how to use the results

API Doc

• This is great. Make sure to reference it in the rest of the documentation. For example, you should reference the API doc for WecOptTool.geom.Parametric when you're describing the geometry parameters. This will add clarity to the document.
• We had a lots of issues implementing the API documentation for MHKiT-MATLAB when we had the source code on master and the documentation on gh-pages. I'm curious how you handled that.

Tests

• How are you running your unit tests? Do you have a runner on Jenkin? @zmorrell-sand just set up a Jenkins server with MATLAB to run WEC-Sim unit tests if you want to do the same thing for this tool

General

• I have a lot of questions about the overall theory/application of the code. It seems like it could be a very useful tool once a general geometry has been decided upon, and someone wants to refine its general dimension. Is that the intended use of this tool? This should be explained up front and be very clear. Users need to know why they would want to use this, and what it can accomplish.

[kmruehl]

Thanks everyone for your feedback. My fault about the loss of genpath. Casual change because it wasn't needed in Windows. I'll try and address all your notes over the weekend. Thanks again. T

@H0R5E I'm running windows and it was required for me.

[H0R5E]

Ok, thanks. I guess its also a backwards compatibility thing then. For my version it just picks up package directories automatically. Ill pop it back in as it wont do any harm. We should definitely make a note as to the oldest version we know to be operable.

[kmruehl]

I was able to install and run on windows, Matlab 2019a. However, my run of example had "Solver Stopped Prematurely" warnings for fmincon.

I was going to recommend against using fmincon because we've had issues with it before as well. I don't remember what the issue was, but I know it's a problematic function.

[kmruehl]

@H0R5E unfortunately MATLAB changes things like that all the time. It's very frustrating, and we have to do version capability checks for MATLAB changes all the time. Recently, for example, array output was changed b/w rows and columns so that messed up all of our matrix calculations. Specifying version compatibility is critical.

I'm running MATLAB 2018b on Windows and I have
Optimization Toolbox Version 8.2 (R2018b)

[kmruehl]

WEC-Sim can conflict with WAFO if they are both on the path at the same time, and since you might share some of the user base, I personally like having to manually add WAFO to my path here rather than a startup file, but I could see the benefits either way.

@dforbush2 did you mean WecOptTool instead of WAFO? I didn't have this issue and I don't think there should be a path issue by adding the WecOptTool path to startup.m. Plus, if people are already using WEC-Sim, adding another path to the startup.m should be a simple addition.

[kmruehl]

It would be great to see the example documentation fleshed out a little more...at least labeling the output figure (I think its power for each of the 8 query sea-states?).

Thinking longer term, it would be great to see a more populated theory/workflow section to get a better idea of whats under the hood and what user I/Os are available. I'm a little concerned about how extendable this meshing approach will be to non-axisymmetric geometries, as Nemoh leaves some to be desired on this front. If you intend to actively support this code I worry you'll have the same issues we do with WEC-Sim where half of our posted issues are actually Nemoh issues.

I wholeheartedly agree with this feedback.

[kmruehl]

okay, I'm out for the weekend. @H0R5E have fun with all of this, sorry if I gave you more than you were asking for :)

[H0R5E]

One minor thing I noticed that may be a good idea to address, the example.m file is dependent on has "use-parallel" option set to True, but this requires the Parallel Computing Toolbox if I am not mistaken. I am unsure of whether MATLAB will default to the non-parallel case if the user does not have the toolbox. We specify in the README and html documentation that the Parallel Optimization Toolbox is optional. I looked into it a little bit, and it seems that license('test', 'Distrib_Computing_Toolbox') can test if someone has access to the toolbox, so perhaps we should place the options inside of an if statement depending on this command?

@zmorrell-sand, thanks this is a really good point. I've opened this one in #83.

As Kelley mentioned, addpath(genpath('/path/to/wecopttool/')) needs to be used on Linux, or installNemoh will not run properly. Other than that, everything runs well out of the box on Linux as well

@zmorrell-sand and @kmruehl thanks, I've got this one TODO in #84 now.

[H0R5E]

@kmruehl, I'm going to reply inline

Okay, I'm going to provide my feedback in a few comments. This is what I have thus far.

Index

• Add a more detailed description about what the WecOptTool is and what it's used for. Why do people want to use this tool? What can it do for them? Why should they bother installing/using this toolbox? A user may think it looks like a lot of work to install, and they don't know why they would want to use it.

Yeah, I think we need this.

• Contents/User Guide headers are redundant

Agreed, I have this structure to allow for multiple major sections (like User vs Technical) but it's not needed right now.

Installation

• "Stable or development versions are available." Where is development, in a dev branch? On a fork? Will development be on master, so the "stable" version is the tagged release? How do I contribute do its development?

Contribution instructions are in the README, and we should add a note pointing people there for interested parties

• I recommend using the .. Note syntax for notes
• Specify version of the dependency, will the MATLAB Optimization Toolbox from 2015b work? I recommend specifying the version you've run as a minimum requirement

Yep, this is important.

• The first step to remove an existing installation is confusing. I recommend making that a note. First step should be download the code via a clone or download.

OK, we'll take another look at this, thanks. Bad things happen if you install two versions.

• For WEC-Sim we have a 'wecsimstartup.m' file that adds the path, see documentation here, you may consider having a startup file in the distribution with a relative path to add the path for the toolbox. You could also check for dependencies. Path issues are one the biggest issues we see, so the easier you can make it on users, the better.

Personally, I don't think startup files are any more reliable given the user may wish to change the startup directory depending on what they are working on at any one time. If there were a single unified location where these could be placed then that would be more useful, but the user still has to do some stuff. Our plan is take the next step in functionality with #58.

• Should be addpath(genpath('C:\Users\kmruehl\Documents\GitHub\WecOptTool\wecopttool_v01_rc3\wecopttool')), otherwise subfolders are not added to directory.

See #84.

• The savepath command gave me a weird warning. May consider having users create a 'startup.m' file in their MATLAB path that gets executed every time MATLAB is opened instead of saving the path

What did the warning say? As I said, not sure I see the relative value of a startup file.

• update documentation to specify correct NEMOH bin file directory, it's C:\Users\kmruehl\Documents\GitHub\Nemoh\Release, not Nemoh\bin

The documentation does give specific Windows vs Linux instructions, but we can try to make them more distinct by rolling them up.

• Until I added the genpath, I got this error, Undefined variable "WecOptLib" or class "WecOptLib.utils.getUserPath". when trying to install nemoh. I recommend having an output of the 'installNemoh' function that tells users the install was successful.
• I like the dependencyCheck functionality, but I think it should be added to the end of the NEMOH install function and run automatically.

This is the subject of #69. I'll mark it high priority.

• I like the runTests functionality, and the command window output it nice, but the documentation should also tell me what a successful 'runtests' looks like.
  Looks like I got a successful run
  Totals: 27 Passed, 0 Failed, 0 Incomplete.

Agreed. Thanks.

Copyright/License

• Why was GNU open-source license selected?
  This will likely be limiting to developers who want to customize the toolbox for their application.

See the discussion in #60

• Where is the Copyright assertion in the LICENSE file?
  <program> Copyright (C) <year> <name of author> This program comes with ABSOLUTELY NO WARRANTY; for details type 'show w'. This is free software, and you are welcome to redistribute it under certain conditions; type 'show c' for details.
<one line to give the program's name and a brief idea of what it does.> Copyright (C) <year> <name of author>

I'm not sure you need to add this to the LICENSE file for GPL as every source file carries it and references to the text in licence file. The README should carry it though.

This should go through Sandia legal and have an NTESS assertion.

@ryancoe has this been done?

• Copyright assertion also needs to be added to documentation:
  /wecopttool_v01_rc3/html_docs/user/license.html

I think the verbatim licence file should be replaced with the source code boilerplate.

• Copyright assertion in source code should not be to "Sandia National Labs". This is what the assertion should look like:
  Copyright 2014 National Technology & Engineering Solutions of Sandia, LLC (NTESS). Under the terms of Contract DE-NA0003525 with NTESS, the U.S. Government retains certain rights in this software.

@ryancoe can you check this? We'll update it (in all the files - urg) if we have to.

Thanks again Kelley. I've summarised these in #85 and #86.

[H0R5E]

Okay, and here's my second round of comments

Right, I've put all your notes regarding content in the example into #87. Many thanks. I'll just address some of the other operational things you posted.

Example

• My MATLAB Workspace has a lot of variables in it. Is there an output object? If so, which one is it? Consider clearing all internal toolbox variables, and only keeping the inputs/ouputs in the workspace upon completed run

I think this is a drawback of having example.m as a script (but this is the most likely way it will be used) and I don't think it should be outputting anything that isn't in example.m. Are you seeing any variables being created that are not defined in example.m?

• Why not use MHKiT instead of WAFO? This is also developed through WPTO funding and we can modify it as need be to support this program. It would be nice to see WPTO funded projects leverage other WPTO funded projects.

Good suggestion! Opened in #88.

• The plot shows up as Figure 29, even though I have no other figures open.

Yeah, I've seen this too. Needs investigating. Opened in #89.

API Doc

• We had a lots of issues implementing the API documentation for MHKiT-MATLAB when we had the source code on master and the documentation on gh-pages. I'm curious how you handled that.

So the plan is to store the sphinx source on master and push the build to gh-pages. You can follow #82 for the implementation until its merged. The next step is to automatically build the docs on a master branch merge. See #72 and #81.

Tests

• How are you running your unit tests? Do you have a runner on Jenkin? @zmorrell-sand just set up a Jenkins server with MATLAB to run WEC-Sim unit tests if you want to do the same thing for this tool

We are just running them manually at the moment and posting the run_tests.pdf file to our PRs for validation. It would be nice to get this running automagically, for sure.

General

• I have a lot of questions about the overall theory/application of the code. It seems like it could be a very useful tool once a general geometry has been decided upon, and someone wants to refine its general dimension. Is that the intended use of this tool? This should be explained up front and be very clear. Users need to know why they would want to use this, and what it can accomplish.

Yeah, I think there is a balance of enticing without over-selling. This is just a beta, so we don't want to promise too much and not deliver otherwise people might not come back [ to DTOcean :'( ]. @ryancoe what's your thoughts here?

[H0R5E]

@dforbush2, thanks for your feedback. I'll reply inline:

I was able to install and run on windows, Matlab 2019a. However, my run of example had "Solver Stopped Prematurely" warnings for fmincon.

Yes, we deliberately stop the optimiser so that the example runs quicker. We need to make a note about this, so I've opened #90.

Code is mostly clear. Its like Matlab wearing a python costume.

Well, you know, they are pretty much the same programming language.

WEC-Sim can conflict with WAFO if they are both on the path at the same time, and since you might share some of the user base, I personally like having to manually add WAFO to my path here rather than a startup file, but I could see the benefits either way.

See my comments to Kelley on this.

I think for a version 0.1 release you guys look good!

Many thanks!

It would be great to see the example documentation fleshed out a little more...at least labeling the output figure (I think its power for each of the 8 query sea-states?).

Thanks, I've popped that comment into #87.

Thinking longer term, it would be great to see a more populated theory/workflow section to get a better idea of whats under the hood and what user I/Os are available.

I agree that some technical docs alongside would be great. Depends a bit on resources, I guess, and that is @ryancoe's burden.

I'm a little concerned about how extendable this meshing approach will be to non-axisymmetric geometries, as Nemoh leaves some to be desired on this front. If you intend to actively support this code I worry you'll have the same issues we do with WEC-Sim where half of our posted issues are actually Nemoh issues.

I guess it would be nice if we could be agnostic regarding the solver we called. This somewhat relates to an issue I am going to open regarding generalising mesh creation and it would be nice to be able deliver meshes to other tools that could give compatible results. If we could get it to work then it might well be useful beyond the scope of this toolbox.

[H0R5E]

I was able to install and run on windows, Matlab 2019a. However, my run of example had "Solver Stopped Prematurely" warnings for fmincon.

I was going to recommend against using fmincon because we've had issues with it before as well. I don't remember what the issue was, but I know it's a problematic function.

Not sure about this one. @ryancoe, @ssolson, @zmorrell-sand have you had any bother with this in production runs? It is used in the control optimisation too, so it would be bad if it's not reliable.

[kmruehl]

@H0R5E I'm going to try to reply inline as well, we will see how it goes

@kmruehl, I'm going to reply inline

Okay, I'm going to provide my feedback in a few comments. This is what I have thus far.

Index

• Add a more detailed description about what the WecOptTool is and what it's used for. Why do people want to use this tool? What can it do for them? Why should they bother installing/using this toolbox? A user may think it looks like a lot of work to install, and they don't know why they would want to use it.

Yeah, I think we need this.

• Contents/User Guide headers are redundant

Agreed, I have this structure to allow for multiple major sections (like User vs Technical) but it's not needed right now.

Installation

• "Stable or development versions are available." Where is development, in a dev branch? On a fork? Will development be on master, so the "stable" version is the tagged release? How do I contribute do its development?

Contribution instructions are in the README, and we should add a note pointing people there for interested parties

• I recommend using the .. Note syntax for notes
• Specify version of the dependency, will the MATLAB Optimization Toolbox from 2015b work? I recommend specifying the version you've run as a minimum requirement

Yep, this is important.

• The first step to remove an existing installation is confusing. I recommend making that a note. First step should be download the code via a clone or download.

OK, we'll take another look at this, thanks. Bad things happen if you install two versions.

• For WEC-Sim we have a 'wecsimstartup.m' file that adds the path, see documentation here, you may consider having a startup file in the distribution with a relative path to add the path for the toolbox. You could also check for dependencies. Path issues are one the biggest issues we see, so the easier you can make it on users, the better.

Personally, I don't think startup files are any more reliable given the user may wish to change the startup directory depending on what they are working on at any one time. If there were a single unified location where these could be placed then that would be more useful, but the user still has to do some stuff. Our plan is take the next step in functionality with #58.

That's a fair point, and ultimately a decision up to the code deveopment team. The <Documents/MATLAB/startup.m> file is automatically executed when MATLAB is opened, so modifying it and adding the code's local /source path is the approach we took for WEC-Sim.

• Should be addpath(genpath('C:\Users\kmruehl\Documents\GitHub\WecOptTool\wecopttool_v01_rc3\wecopttool')), otherwise subfolders are not added to directory.

See #84.

• The savepath command gave me a weird warning. May consider having users create a 'startup.m' file in their MATLAB path that gets executed every time MATLAB is opened instead of saving the path

What did the warning say? As I said, not sure I see the relative value of a startup file.

It was a warning about having admin rights on the machine, I just hit accept. My concern with saving the path is that if you change the path, you'll have to remove the current path and and then add a new one (otherwise you'll have both versions of the code in your path). That's the reason we chose to add the path every time MATLAB is opened by modifying the <Documents/MATLAB/startup.m> file, instead of saving it. Once again, this is just a code development preference comment.

• update documentation to specify correct NEMOH bin file directory, it's C:\Users\kmruehl\Documents\GitHub\Nemoh\Release, not Nemoh\bin

The documentation does give specific Windows vs Linux instructions, but we can try to make them more distinct by rolling them up.

Oh, is the Nemoh\bin the name of the LINUX directory? For Windows, it's Nemoh\Release

• Until I added the genpath, I got this error, Undefined variable "WecOptLib" or class "WecOptLib.utils.getUserPath". when trying to install nemoh. I recommend having an output of the 'installNemoh' function that tells users the install was successful.
• I like the dependencyCheck functionality, but I think it should be added to the end of the NEMOH install function and run automatically.

This is the subject of #69. I'll mark it high priority.

• I like the runTests functionality, and the command window output it nice, but the documentation should also tell me what a successful 'runtests' looks like.
  Looks like I got a successful run
  Totals: 27 Passed, 0 Failed, 0 Incomplete.

Agreed. Thanks.

Copyright/License

• Why was GNU open-source license selected?
  This will likely be limiting to developers who want to customize the toolbox for their application.

See the discussion in #60

• Where is the Copyright assertion in the LICENSE file?
  <program> Copyright (C) <year> <name of author> This program comes with ABSOLUTELY NO WARRANTY; for details type 'show w'. This is free software, and you are welcome to redistribute it under certain conditions; type 'show c' for details.
<one line to give the program's name and a brief idea of what it does.> Copyright (C) <year> <name of author>

I'm not sure you need to add this to the LICENSE file for GPL as every source file carries it and references to the text in licence file. The README should carry it though.

This should go through Sandia legal and have an NTESS assertion.

@ryancoe has this been done?

• Copyright assertion also needs to be added to documentation:
  /wecopttool_v01_rc3/html_docs/user/license.html

I think the verbatim licence file should be replaced with the source code boilerplate.

• Copyright assertion in source code should not be to "Sandia National Labs". This is what the assertion should look like:
  Copyright 2014 National Technology & Engineering Solutions of Sandia, LLC (NTESS). Under the terms of Contract DE-NA0003525 with NTESS, the U.S. Government retains certain rights in this software.

@ryancoe can you check this? We'll update it (in all the files - urg) if we have to.

Thanks again Kelley. I've summarised these in #85 and #86.

You're welcome, hope this feedback helps!

[kmruehl]

@H0R5E Attempting to respond in line here too...

Okay, and here's my second round of comments

Right, I've put all your notes regarding content in the example into #87. Many thanks. I'll just address some of the other operational things you posted.

Thank you, I'll review issue #87

Example

• My MATLAB Workspace has a lot of variables in it. Is there an output object? If so, which one is it? Consider clearing all internal toolbox variables, and only keeping the inputs/ouputs in the workspace upon completed run

I think this is a drawback of having example.m as a script (but this is the most likely way it will be used) and I don't think it should be outputting anything that isn't in example.m. Are you seeing any variables being created that are not defined in example.m?

I added a screenshot of my MATLAB workspace to the comment, it may have been added after you saw the comments (I made a few minor revisions). I'm not sure if all of the variables are defined in the example.m, they probably are. Either way, I would recommend having an output object that clearly specifies the relevant I/O parameters and provide users with guidance on how to interact with that object. This is likely beyond the scope of an alpha release, but I think it would be very useful for users. Right now, after running the example, it was unclear what the result was and how to interact with it.

• Why not use MHKiT instead of WAFO? This is also developed through WPTO funding and we can modify it as need be to support this program. It would be nice to see WPTO funded projects leverage other WPTO funded projects.

Good suggestion! Opened in #88.

• The plot shows up as Figure 29, even though I have no other figures open.

Yeah, I've seen this too. Needs investigating. Opened in #89.

API Doc

• We had a lots of issues implementing the API documentation for MHKiT-MATLAB when we had the source code on master and the documentation on gh-pages. I'm curious how you handled that.

So the plan is to store the sphinx source on master and push the build to gh-pages. You can follow #82 for the implementation until its merged. The next step is to automatically build the docs on a master branch merge. See #72 and #81.

I'll take a look at #82 . This is an issue we are currently trying to resolve with MHKiT-MATLAB. Right now we are compiling them in master and copying them over to gh-pages, which is a short-term solution, and has obvious drawbacks... including that we lose the index.

Tests

• How are you running your unit tests? Do you have a runner on Jenkin? @zmorrell-sand just set up a Jenkins server with MATLAB to run WEC-Sim unit tests if you want to do the same thing for this tool

We are just running them manually at the moment and posting the run_tests.pdf file to our PRs for validation. It would be nice to get this running automagically, for sure.

Okay, let me/@zmorrell-sand how we can help. We have Jenkins set up for WEC-Sim, the server is on a virtual machine with MATLAB installed. It should be relatively simple to set up a Jenkins webhook for the WecOpTTool

General

• I have a lot of questions about the overall theory/application of the code. It seems like it could be a very useful tool once a general geometry has been decided upon, and someone wants to refine its general dimension. Is that the intended use of this tool? This should be explained up front and be very clear. Users need to know why they would want to use this, and what it can accomplish.

Yeah, I think there is a balance of enticing without over-selling. This is just a beta, so we don't want to promise too much and not deliver otherwise people might not come back [ to DTOcean :'( ]. @ryancoe what's your thoughts here?

Understood. But if your goal is to target "WEC developers and researchers" you have to tell them why they care, and provide them the relevant theory/application. Similar comment as before, this is something that could/should be built upon for a future release. In my eyes, the goal of an alpha release is to get something functional out the door, examples/documentation are ongoing works in progress...

[ryancoe]

Thanks @kmruehl @zmorrell-sand @dforbush2! - I'm going to close out this issue as the key items have be split out into individual issues.

[H0R5E]

My concern with saving the path is that if you change the path, you'll have to remove the current path and and then add a new one (otherwise you'll have both versions of the code in your path).

@kmruehl, I see where you are coming from here. For us, this would only be an issue for those using the "development version" of the code, i.e. working from a cloned copy which they update. I also think its a legacy issue (although it seems to carry over to linux for some reason) because we should only need to add one folder (toolbox) and the package directories below should be automatically found (according to https://www.mathworks.com/help/matlab/matlab_oop/scoping-classes-with-packages.html#brfynt_-3).