#5 Now matlab scripts run correctly with modern matlab #6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
sedurCode
added a commit
to sedurCode/carfac
that referenced
this pull request
Mar 1, 2021
# This is the 1st commit message: 9 update cpp build settings and gtest compatability # This is the commit message google#2: Revert back to test suite when using a more uptodate version of gtest than supplied by apt-get # This is the commit message google#3: actually version 1.8 # This is the commit message google#4: scons instructions update # This is the commit message google#5: google#5 Now matlab scripts run correctly with modern matlab # This is the commit message google#6: Update SConstruct to be python 3 compatible.
sedurCode
added a commit
to sedurCode/carfac
that referenced
this pull request
Mar 1, 2021
# This is the 1st commit message: 9 update cpp build settings and gtest compatability # This is the commit message google#2: Revert back to test suite when using a more uptodate version of gtest than supplied by apt-get # This is the commit message google#3: actually version 1.8 # This is the commit message google#4: scons instructions update # This is the commit message google#5: google#5 Now matlab scripts run correctly with modern matlab # This is the commit message google#6: Update SConstruct to be python 3 compatible. # This is the commit message google#7: revert matlab-related changes # This is the commit message google#8: whitespace
dicklyon
requested changes
Mar 1, 2021
|
Yes, it's OK, either as is or by rectifying; nap is real, but not quite
nonnegative as expected (due to the highpass in the IHC, I suppose).
image(63 * max(0, smooth_nap)' .^ 0.5)) % Rectify since nap can go
slightly negative.
…On Tue, Mar 2, 2021 at 4:30 AM Simon Durbridge ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In matlab/CARFAC_binaural.m
<#6 (comment)>:
>
[CF_struct, nap_decim, nap] = CARFAC_Run(CF_struct, test_signal, agc_plot_fig_num);
% Display results for 2 ears:
for ear = 1:n_ears
smooth_nap = nap_decim(:, :, ear);
figure(ear + n_ears) % Makes figures 3 and 4
- image(63 * ((smooth_nap)' .^ 0.5))
+ image(63 * abs((smooth_nap)' .^ 0.5))
I need to dig a little deeper, but Im guessing for the meantime its
probably ok to rectify negative nap values?
We aren't modelling actual neuron firing action potentials.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#6 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGW7DDSKY6JCOTAA3NN6EDTBTK6NANCNFSM4XOSHLQA>
.
|
Change added. |
|
All good by me. Thanks. |
sedurCode
force-pushed
the
fix-for-modern-matlab
branch
from
f2feaee
to
5ceca36
Compare
sedurCode
force-pushed
the
fix-for-modern-matlab
branch
from
e12ceef
to
b44ee4c
Compare
|
Squashed to one commit. |
|
Hi @dicklyon I've just had a look at this stale PR from a couple years ago; Is this still valid, and are the changed still valid, or shall I close this PR? |
|
Simon, I'll try to get one of my software engineers to figure this out.
Thanks.
Dick
…On Thu, May 18, 2023 at 7:16 AM Simon Durbridge ***@***.***> wrote:
Hi @dicklyon <https://github.com/dicklyon> I've just had a look at this
stale PR from a couple years ago; Is this still valid, and are the changed
still valid, or shall I close this PR?
—
Reply to this email directly, view it on GitHub
<#6 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGW7DE6YHNW5PE2I4YCAX3XGYVKFANCNFSM4XOSHLQA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
yuhonglin
reviewed
May 19, 2023
Comment on lines
+44
to
+45
| % Rectify since nap can go slightly negative. | ||
| image(63 * (max(0, smooth_nap)' .^ 0.5)); |
There was a problem hiding this comment.
Dick: should we rectify NAP like this or shift it?
There was a problem hiding this comment.
Just saw Dick's comment in another thread, so this should be OK.
Comment on lines
+22
to
+29
| clear_old_files = true; | ||
|
|
||
| [status, msg, msgID] = mkdir('frames'); | ||
|
|
||
| if (status && clear_old_files && strcmp(msgID, 'MATLAB:MKDIR:DirectoryExists')) | ||
| disp("Clearing old frames"); | ||
| delete('frames/*'); | ||
| end |
There was a problem hiding this comment.
This changes the behavior of previous code: the previous code always deletes all the images but this one won't delete the images after the computation. Maybe let's move the deletion to the end of the computation?
yuhonglin
reviewed
May 19, 2023
| %% | ||
| system('rm -r frames'); | ||
|
|
||
| wav_fn, [wav_fn(1:end-4), '.mpg']) |
There was a problem hiding this comment.
Can we use something like fileparts to bring back the CARFAC_SAI_movie_ prefix? https://www.mathworks.com/help/matlab/ref/fileparts.html
|
I don't really know what facilities matlab has for file path handling, but
sounds like a good idea.
Dick
…On Fri, May 19, 2023 at 2:49 AM Honglin Yu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In matlab/CARFAC_SAI_hacking.m
<#6 (comment)>:
> @@ -64,9 +71,5 @@
%%
png_name_pattern = 'frames/frame%05d.png';
MakeMovieFromPngsAndWav(round(frame_rate), png_name_pattern, ...
- wav_fn, ['CARFAC_SAI_movie_', wav_fn(1:end-4), '.mpg'])
-
-%%
-system('rm -r frames');
-
+ wav_fn, [wav_fn(1:end-4), '.mpg'])
Can we use something like fileparts to bring back the CARFAC_SAI_movie_
prefix? https://www.mathworks.com/help/matlab/ref/fileparts.html
—
Reply to this email directly, view it on GitHub
<#6 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGW7DBRBW7W5K7BZIOD3MDXG46Y7ANCNFSM4XOSHLQA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Now that I have more time, I'll try to address your comments in a following commit |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the three matlab scripts to work with modern matlab.
The gitignore is also set up to ignore the common temporary files of matlab and python.
It is assumed that however you install ffmpeg by Brew, apt or downloaded binary, that ffmpeg will end up on the system path, therefore we are no longer 'locked' to
opt/lib/usr...Scripts should now perform the correct system calls on unix, mac and windows.