Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default sprectral weighting #137

Closed
wants to merge 5 commits into from
Closed

Default sprectral weighting #137

wants to merge 5 commits into from

Conversation

rgcoe
Copy link
Member

@rgcoe rgcoe commented May 5, 2020

Description

I found the toggling of warnings in the example file particularly offensive for some reason :), so I made changes to set the equal weighting of the example spectra. This relates to #108, although does not close it.

Fixes (none)

test_results.pdf

Checklist:

  • Added the results of running the test suite (in pdf form)
  • All new files contain the GPL header
  • If example.m has been modified, the content / line numbers in
    docs\user\example.rst are still valid or have been fixed

@rgcoe rgcoe requested a review from H0R5E May 5, 2020 16:50
@H0R5E
Copy link
Member

H0R5E commented May 7, 2020

I think this is a good idea, as it looks a bit shonky having an example that triggers our own warning. Before I review this, though, I would like to suggest a couple of things to add value to the PR:

  1. Take the opportunity to move this warning outside of the optimiser by moving lines 146-154 of DeviceModelTemplate.m into the addSpectra method of RM3Study.m (between lines 91 and 92). Then update the error message on line 158 of DeviceModelTemplate.m to just say you need to set mu throughout. Some of the tests might need updated to add mu to the input spectra.
  2. Maybe update 8spectra.mat rather than add code to the loading function, just to avoid confusion down the line if someone adds a different 8spectra.mat and doesn't get that mu is being overwritten.

I'm not totally sold on the second suggestion, but I think the first one would be a useful improvement from this PR, beyond tidying up the example.

@rgcoe
Copy link
Member Author

rgcoe commented May 7, 2020

I completed 2 with cf7747c and a version of 1 with 705da40.

"version of 1:" I moved the checking for mu to WecOptLib.utils.checkSpectrum, which was already being called in RM3Study.addSpectra. I do not check the spectral weighting within DeviceModelTemplate.m, because it seems like this would be checking them twice (right?).

Additionally, I added some unit tests for this functionality to checkSpectrumTest.

test_results.pdf

@H0R5E
Copy link
Member

H0R5E commented May 13, 2020

I'll try and get the review of this done before our meeting tomorrow.

Copy link
Member

@H0R5E H0R5E left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so there is an issue that checkSpectrum doesn't return the fixed spectrum, so when you run the example with a WAFO generated spectra it bugs out:

Running study...
Reference to non-existent field 'mu'.

Error in WecOptLib.models.DeviceModelTemplate/getPower (line 155)
            mus(iSS) = S.mu;

Error in WecOptTool.run/obj (line 69)
        pow = -1 * RM3Device.getPower(study.studyDir,           ...

Error in fmincon (line 562)
      initVals.f = feval(funfcn{3},X,varargin{:});

Error in WecOptTool.run (line 93)
    [sol,fval,exitflag,output] = fmincon(@obj,                  ...

Error in example (line 62)
WecOptTool.run(study, options);

Caused by:
    Failure in initial objective function evaluation. FMINCON cannot continue. 

The name checkSpectrum has also become a bit weird in that it fixes the spectra as well as checks them. My personal preference would be to have checkWeightingsPresent as standalone function rather than rolling it into checkSpectrum and change its name to be clearer that it's going to change the input.

@@ -21,9 +21,12 @@
function SS = example8Spectra
%EXAMPLE8SPECTRA Example Bretschneider spectrum with varying HHm0s, Tps,
% Nbins, and range

% load data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a superfluous comment. Remove or edit the docstring, if necessary.

inds = 1:length(S);

% check if there are no weightings, if necessary populate and warn
S = checkWeightingsPresent(S);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this to be useful S must be returned by checkSpectrum

% S.S column vector of spectral density in [m^2 rad/ s]
%
% Outputs
% (none) (will throw error if not valid)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would need changed if S is returned

% WecOptTool is distributed in the hope that it will be useful,
% but WITHOUT ANY WARRANTY; without even the implied warranty of
% MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
% GNU General Public License for more details.
%
%
% You should have received a copy of the GNU General Public License
% along with WecOptTool. If not, see <https://www.gnu.org/licenses/>.

function [] = checkSpectrum(S)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if checkSpectrum is the correct name for this now if it does two jobs, checking and fixing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, but I actually do prefer that its just one function call to do both

Copy link
Member

@H0R5E H0R5E May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I am going to push you on this, but not just because it's nasty to have one function with two purposes (ick). I'm most worried that, as we change to the new structure, there is a risk that this function ends up back inside the optimisation loop without the spectra being corrected first, rendering the original purpose of stopping the warning showing up in every iteration moot.

If we have an explicit function that fixes the mu prior to use in any optimisation that the user creates, then checkSpectrum can simply generate an error if it's not set (for more than one spectrum), which is its typical behaviour, allowing it to be safely used anywhere in the code. Does that make sense?

end
end

function [S] = checkWeightingsPresent(S)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making this a standalone function, then checkSpectrum doesn't need to return S and it's purpose remains clear. I think the name could be changed as well, to something like "fixWeightings", just to show that it changes the input.

@rgcoe
Copy link
Member Author

rgcoe commented Jun 25, 2020

Based on #143 and #144, we can close this by simply updating the .mat files for the example spectra. Also the warnings would happened only once (outside of loop), so not a big deal anyways.

@H0R5E H0R5E self-assigned this Aug 6, 2020
@H0R5E
Copy link
Member

H0R5E commented Aug 6, 2020

.mat files added to #161

@H0R5E H0R5E closed this Aug 6, 2020
@rgcoe rgcoe deleted the defaultSprectralWeighting branch September 25, 2020 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants