MHKiT-Software
Acoustics Module
Functions and example notebook for reading passive acoustics data from hydrophones.
@jmcvey3 thanks for making this module! I fixed the ipython nb merge issue so all tests pass but now your tests are going to fail due to pylint. As a new module passing pylint is required as well as type hints in all function inputs. Could you address the pylint and type hint concerns?
@jmcvey3 Can you add a PR to the documentation repo adding this new module to the API?
@ssolson Would recommend ignoring pylint codes R913, R914, R915 and E1101 if you want the pylint tests to pass.
@ssolson Would recommend ignoring pylint codes R913, R914, R915 and E1101 if you want the pylint tests to pass.
Let me see if I can address.
Today I confirmed pylint E1001 should be disabled. Pylint believes the file is being opened in read mode:
mhkit\acoustics\io.py:356:8: E1101: Instance of 'Wave_read' has no 'setnchannels' member; maybe 'getnchannels'? (no-member)
mhkit\acoustics\io.py:357:8: E1101: Instance of 'Wave_read' has no 'setsampwidth' member; maybe 'getsampwidth'? (no-member)
mhkit\acoustics\io.py:358:8: E1101: Instance of 'Wave_read' has no 'setframerate' member; maybe 'getframerate'? (no-member)
mhkit\acoustics\io.py:359:8: E1101: Instance of 'Wave_read' has no 'writeframes' member (no-member)
However:
print(type(f)) returns <class 'wave.Wave_write'>
This confirms the issue is with pylint incorrectly interpreting the object as Wave_read type.
What would you like to do about the "R" error codes? The number of variables and lines used per function are arbitrary at best.
What would you like to do about the "R" error codes? The number of variables and lines used per function are arbitrary at best.
I am going to try to address the too many locals by using a dictionary.
I am still thinking about the too many arguments. What do you think about keeping the current way the function works but adding the ability for the user to pass a dictionary e.g. {'quantile': 0.25} or {'min': None} ect.?
I have not looked into the too many statements yet.
What would you like to do about the "R" error codes? The number of variables and lines used per function are arbitrary at best.
I am going to try to address the too many locals by using a dictionary.
I am still thinking about the too many arguments. What do you think about keeping the current way the function works but adding the ability for the user to pass a dictionary e.g. {'quantile': 0.25} or {'min': None} ect.?
I have not looked into the too many statements yet.
A dictionary could work in some cases, and so could updating a variable name. I think it becomes a case of "when am I making the code harder to read or more complicated than it needs to be for the sake of an arbitrary test?". Solving "too many variables" will probably break "too many statements" and/or vice versa.
A better coded way of solving "too many arguments" is to use *args and **kwargs, but these concepts may be difficult for new users to understand.
What would you like to do about the "R" error codes? The number of variables and lines used per function are arbitrary at best.
I am going to try to address the too many locals by using a dictionary. I am still thinking about the too many arguments. What do you think about keeping the current way the function works but adding the ability for the user to pass a dictionary e.g. {'quantile': 0.25} or {'min': None} ect.? I have not looked into the too many statements yet.
A dictionary could work in some cases, and so could updating a variable name. I think it becomes a case of "when am I making the code harder to read or more complicated than it needs to be for the sake of an arbitrary test?". Solving "too many variables" will probably break "too many statements" and/or vice versa.
A better coded way of solving "too many arguments" is to use
*argsand**kwargs, but these concepts may be difficult for new users to understand.
I resolved the issues without ignores in a new PR on your branch. I need to write tests still for some of the functions I broke out.
Dictionaries make the code harder to read imo but should also force you to group like variables.
In my experience trying to follow pylint is annoying, and takes longer but ultimately makes the code more modular and easier to work with long term. We're fully the realm of preferences with the "Refactor" codes so there is no right or wrong. My goal is to not use inline ignores unless there is no other way. Then with global ignores I think it is hard to justify that there should not be some limit on too many locals and too many arguments. So at that point it just boils down to what the limit should be and again there is no right answer.
@ssolson Git wouldn't let me push to your branch, so I merged your PR and then committed my changes. A couple "dry" changes, and some other edits that are relatively minor but better for readability.
@jmcvey3 I'm happy with the io module as I went over it in the previous round.
Can I get a quick update on where you are with this PR and if you are good to merge with one before #354?
@jmcvey3 I'm happy with the io module as I went over it in the previous round.
Can I get a quick update on where you are with this PR and if you are good to merge with one before #354?
I am ready to merge when you are. Looks like the wave example notebook got messed up again though