Improve input syntax for frequency differencing operation

[leewujung]

Currently the frequency differencing implementation takes an awkward input that forces users to separate out the channels or frequencies from the operators (>, < , etc) that operates on them. I think the frequency differencing criteria (something like chanA - chanB >= X dB or freqA - freqB >= X dB) is something that can be done with regular expression and enhances the usability significantly.

Requirements

• parse the input arguments (chanA/B or freqA/B (do not mix use of chan and freq), X dB, >= or other comparison operators) directly from the criteria equation
• add tests

[praneethratna]

Hey @leewujung, Sorry for the delay, it took some time for development setup due to issues with setting up docker. I have a query regarding this issue - So now there should be two arguments where one equation is in terms of chan and other equation in terms of freq and only one of them should be provided?

def frequency_differencing(
    source_Sv: Union[xr.Dataset, str, pathlib.Path],
    storage_options: Optional[dict] = {},
    freqABDiff: Optional[str] = None,
    chanABDiff: Optional[str] = None,
) -> xr.DataArray:

Is this the correct function definition according to the change? Also can i be assigned this issue?

[leewujung]

Hey @praneethratna : no worries, thanks for contributing! I just assigned you to the issues.

Yes I was thinking to have something like this:

def frequency_differencing(
    source_Sv: Union[xr.Dataset, str, pathlib.Path],
    diff_eq_freq: Optional[str] = None,  # eg "38 kHz - 18 kHz >= 15 dB"
    diff_eq_chan: Optional[str] = None,  # eg "WBT 743366-15 ES38B_ES - WBT 743367-15 ES18_ES < 30 dB"
    storage_options: Optional[dict] = {},
)

diff_eq_freq or diff_eq_chan can be used, but not both.
In the above, strings like "WBT 743366-15 ES38B_ES" and "WBT 743367-15 ES18_ES" are values found in the channel dimension in the Sv dataset (there are almost always spaces in the channel strings).
I think we should also be able to deal with arbitrary spaces like "38kHz -18kHz>=15 dB".

Did the above answer your question?

[praneethratna]

Did the above answer your question?

Hey @leewujung, yes and one small doubt - in the freq equation do we consider freqB as -18 khz or it is 18 khz?

[leewujung]

It is 18 kHz (and that's that ES18 in the channel string, similarly for 38 kHz)

[praneethratna]

Got it!, will make a PR as soon as I'm done finishing with the changes.

[praneethratna]

Hey @leewujung Sorry if I'm asking too many question as I'm new to sonar data but can the unit of freqA/B vary in terms of Hz something like MHz or GHz or is it only kHz and similarly for diff as well?

[leewujung]

Oh no worries at all! That's a great question... For the instruments echopype supports at the moment, it will always be kHz, but yes for sonar systems in general it can be others, like Hz, MHz, GHz. For the difference, we can assume to just allow X dB, and X can be a negative or positive number, or 0.

[praneethratna]

@leewujung Cool! Got it, So I'll make changes such that freqA/B can be anything like kHz,MHz etc and diff being only in dB.

[praneethratna]

Hey @leewujung, what is the format of channel names? Do they always end with ES or is it arbitrary?

[leewujung]

It is somewhat arbitrary. For the echosounders we support now, you can take a look at the values in the channel dimension (under Sonar > Beam_groupX ) in this doc update that's current in a PR for the variation. But it is safer to assume it is arbitrary. I wonder if we should ask user to put in double quote (like "CHANNEL_STRING") to avoid this being super messy.

[leewujung]

Also, one more thing. The frequency they put in has to be one of the values in frequency_nominal (also see under Sonar > Beam_groupX), as you see in the checks in the original function.

[praneethratna]

It is somewhat arbitrary. For the echosounders we support now, you can take a look at the values in the channel dimension (under Sonar > Beam_groupX ) in this doc update that's current in a PR for the variation. But it is safer to assume it is arbitrary. I wonder if we should ask user to put in double quote (like "CHANNEL_STRING") to avoid this being super messy.

Ohh, it seems channel names need not end with ES. Also, yes i think extracting channel names from something like WBT 743366-15 ES38B_ES - WBT 743367-15 ES18_ES < 30 dB (without double quotes to channel names) will be quite difficult and messy rather than extracting from something like "WBT 743366-15 ES38B_ES" - "WBT 743367-15 ES18_ES" < 30 dB. This will also make the code a lot simpler as well! @leewujung So should i follow that convention and update the docstring accordingly?

[leewujung]

So should i follow that convention and update the docstring accordingly?

That seems a good idea! It is actually easier to read as well. Thanks!

[praneethratna]

@leewujung This can be closed now!

[leewujung]

Oh right! Closing it now as it has been addressed in #1106! :)