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

Impedance type change, name change, bug fix #1019

Merged
merged 8 commits into from
Apr 7, 2023

Conversation

leewujung
Copy link
Member

@leewujung leewujung commented Apr 2, 2023

This PR contains the following changes:

  • change the data type of impedance to float (addresses impedance int > float #1018)
  • finish renaming impedance_transmit to impedance_transducer and impedance_receive to impedance_transceiver (leftover that resulted into bug from v0.7.0)
  • rename transmit chirp replica construction methods:
    • correct typo Anderson to Andersen
    • change Macaulay to Matlab since original code came from Andersen
    • remove the previous Macaulay chirp replica implementation and only keep the one in reference to Andersen

@leewujung leewujung added bug Something isn't working data format Anything about data format labels Apr 2, 2023
@leewujung leewujung added this to the 0.7.1 milestone Apr 2, 2023
@leewujung leewujung requested a review from emiliom April 2, 2023 17:45
@codecov-commenter
Copy link

codecov-commenter commented Apr 2, 2023

Codecov Report

Merging #1019 (8fb39cb) into dev (2736ee9) will decrease coverage by 13.54%.
The diff coverage is 92.30%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##              dev    #1019       +/-   ##
===========================================
- Coverage   80.45%   66.91%   -13.54%     
===========================================
  Files          67       30       -37     
  Lines        6038     3857     -2181     
===========================================
- Hits         4858     2581     -2277     
- Misses       1180     1276       +96     
Flag Coverage Δ
unittests 66.91% <92.30%> (-13.54%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
echopype/convert/utils/ek_raw_parsers.py 40.59% <ø> (-15.31%) ⬇️
echopype/calibrate/ek80_complex.py 86.95% <66.66%> (ø)
echopype/calibrate/cal_params.py 91.66% <100.00%> (ø)
echopype/calibrate/calibrate_ek.py 97.87% <100.00%> (ø)
echopype/convert/set_groups_ek80.py 83.80% <100.00%> (-12.96%) ⬇️

... and 54 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@emiliom
Copy link
Collaborator

emiliom commented Apr 3, 2023

change Macaulay to Matlab since original code came from Andersen

I don't know anything about this, but an implementation name of "Matlab" seems not specific enough, unlike Andersen and Macaulay. The Andersen implementation code has a helpful 2-line comment that provides background. Is the "Matlab" implementation from a specific m-script file, from the general Matlab code base, or something else?

Copy link
Collaborator

@emiliom emiliom left a comment

Choose a reason for hiding this comment

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

Done reviewing. In addition to my other small comments, I mainly checked for consistency of the variable renaming, impedance_transmit -> impedance_transducer and impedance_receive -> impedance_transceiver. I didn't find any errors (inconsistencies).

@leewujung
Copy link
Member Author

Is the "Matlab" implementation from a specific m-script file, from the general Matlab code base, or something else?

The "Matlab" implementation came from Lars Andersen in matlab Echolab code that was passed through multiple people (I got it from Chu).

In fact, I think the best way to do this is to remove this implementation completely. I spoked with Lars about this and the specific chirp implementation is due to the implementation in hardware, so we should stick to it.

@leewujung
Copy link
Member Author

@emiliom : I've added type hint to z_et and z_er. Also added a source reference to the gain correction based on angle offset and beamwidth that was undocumented anywhere else -- I asked Lars about it and added the explanation in the docstring.

Let me know what you think about the "Matlab" implementation of chirp.

@emiliom
Copy link
Collaborator

emiliom commented Apr 4, 2023

The "Matlab" implementation came from Lars Andersen in matlab Echolab code that was passed through multiple people (I got it from Chu).

👍

In fact, I think the best way to do this is to remove this implementation completely. I spoked with Lars about this and the specific chirp implementation is due to the implementation in hardware, so we should stick to it.

Well, the issue goes away if you remove the "Matlab" implementation 😉. So, no additional comment here. It's up to you whether to keep it or not. If you do keep it, my suggestion would be to simply change the label to something like "MatlabEcholab" and add a comment like the sentence above describing its origin.

@leewujung
Copy link
Member Author

leewujung commented Apr 5, 2023

I think I'll opt for removing that section entirely, since it does not have the simplicity of just using a general chirp function, nor does it conform with what Simrad actually uses. I'll push up a commit for the removal.

@leewujung leewujung merged commit dd02d79 into OSOceanAcoustics:dev Apr 7, 2023
@leewujung leewujung mentioned this pull request Apr 7, 2023
@leewujung leewujung deleted the impedance-change branch July 21, 2024 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working data format Anything about data format
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants