Adds the upcrossing analysis functions in MATLAB #151
Conversation
MShabara
changed the title
MShabara
changed the title
There was a problem hiding this comment.
@MShabara, overall the implementation looks good and I don't see any technical errors. Could you please:
- Update the docstrings to follow this projects format
- Address the inline comments I've left about test file locations, unused code, and citations
| methods (TestMethodSetup) | ||
| % Setup for each test | ||
| end | ||
|
|
||
| % methods (Test) | ||
| % % Test methods | ||
| % | ||
| % function unimplementedTest(testCase) | ||
| % testCase.verifyFail("Unimplemented test"); | ||
| % end | ||
| % end |
There was a problem hiding this comment.
Can you move this to examples/data/utils ?
| --- | ||
|
|
||
| ## Author(s) | ||
| - **mshabara** |
There was a problem hiding this comment.
Can we cite the original authors of this code also? https://github.com/MHKiT-Software/MHKiT-Python/blob/bc1eadc9117f969bed618a104834892bff1e3df0/mhkit/utils/upcrossing.py#L16-L19
| % Apply a function `f` over intervals defined by `inds`. If `inds` is | ||
| % not provided, compute the indices using the upcrossing function. | ||
|
|
||
| % Input: | ||
| % t : Array of time values. | ||
| % data : Array of data values. | ||
| % f : Function handle that takes two indices (start, end) and returns a scalar value. | ||
| % inds : Indices array (optional). If not provided, `upcrossing` will be used. | ||
|
|
||
| % Output: | ||
| % vals : Array of values resulting from applying `f` over the intervals defined by `inds`. |
There was a problem hiding this comment.
Docstrings need to follow this projects docstring format, which is used by sphinx-matlabdomain to generate the MHKiT-MATLAB website documentation. For consistency, please reference the format shown in delft_3d_calculate_variable_interpolation.m in the MHKiT-MATLAB repository. This gets rendered here.
At this time I cannot find any specific documentation for this format. I'll update our contributor documentation to include these docstring specifications for future reference.
For this function the docstring should be:
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
%
% Apply a function over defined intervals in time series data.
%
% Parameters
% ------------
% t: array
% Array of time values.
% data: array
% Array of data values.
% f: function handle
% Function that takes two indices (start, end) and returns a scalar value.
% inds: array, optional
% Indices array defining the intervals. If not provided, intervals will be
% computed using the upcrossing function.
%
% Returns
% ---------
% vals: array
% Array of values resulting from applying the function over the defined intervals.
%
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
| end | ||
|
|
||
| n = numel(inds) - 1; % Number of intervals | ||
| vals = zeros(1, n); % Initialize the output array |
There was a problem hiding this comment.
Do you think the vals array should be initialized as zeros? If f fails to return would that leave unintended zeroes in the output? Maybe it would be better to initialize the output vals array as NaNs?
This PR is related to the uncrossing analysis utilities:
1- The upcrossing functions were added.
2- The upcrossing_Test unit test is added.