Add changes by Michael Stanway (April 2023) #51
Conversation
|
This looks manageable - 6 files changed and not 55 like it was in #50! However, we have failed test - needs to be investigated: |
olexandr-konovalov
requested review from
michaelstanway,
vmjanik,
vdeecke and
tatedunbar
olexandr-konovalov
force-pushed
the
refactor-classes
branch
from
924a0ce to
81675a8
Compare
|
Test failed because of Seems that some function changed the number of arguments, but the test was not adjusted. |
NetworkFactory.m
Outdated
| % Creates an empty | ||
| weights = ones(max([contours.length]), 0); | ||
| net = Network(weights); | ||
| function net = new_network() |
There was a problem hiding this comment.
@michaelstanway the tests now failed because they expect an argument given to new_network. Do you know how to fix this? We were puzzled that new_network does not require any arguments in your version.
olexandr-konovalov
force-pushed
the
refactor-classes
branch
from
81675a8 to
1f4a96f
Compare
|
I have rebased this, hoping that we can also see code coverage for this PR - however, the test terminates because of the error prior to uploading results to Codecov. |
| if length(varargin) == 1 | ||
| dir_path = varargin{1}; | ||
| else | ||
| dir_path = cd + "/Test_Data"; % temporarily the subdirectory ./Test_Data | ||
| dir_path = cd + "/BrazilData"; % temporarily the subdirectory ./Test_Data |
There was a problem hiding this comment.
There was no BrazilData subdirectory in the archive I have received from @vmjanik. I will switch back to Test_Data next time we meet. Although this will not help with the other problem, this will demonstrate how to edit PRs.
| @@ -1,19 +1,62 @@ | |||
| classdef NetworkFactory | |||
| % A factory to initialise the network to be used within an ARTwarp run. | |||
| % Summary of this class goes here | |||
There was a problem hiding this comment.
It looks like the text in main is newer than suggested change - perhaps @michaelstanway used earlier version of the code, and in the meantime the comment was added? Of course, we have no way to figure out on which revision in the repository @michaelstanway's work was based, since Michael did not use version control. I am starting to suspect that this PR will be closed without merging, and we will have to manually recover ideas or go over text comments and pick up what is salvageable and helpful.
This PR tries to apply some changes earlier attempted by Michael Stanway and shown in PR #51 but in a accurate way, and also adding some tests -------- Co-authored-by: Tate Dunbar
Attempt to do PR dolphin-acoustics-vip#50 properly.
olexandr-konovalov
force-pushed
the
refactor-classes
branch
from
1f4a96f to
b7c5dbe
Compare
|
Now part of this was added in PR #64 by @tatedunbar which has been merged. After that Iv'e rebased this PR #51, so we can now see what's left. @tatedunbar please check if there is anything else what we need to salvage, and then make another PR if you want to backport more of Michael's changes. Thanks! |
Attempt to do PR #50 properly. Submitted this PR using an archive provided to us by @vmjanik via MS Teams for the Dolphin Acoustics VIP.