meinardmueller / libfmp Goto Github PK

As scipy 1.9 removed hanning window function (release notes), we may change it to hann.

Hi everyone,
I am currently reviewing your JOSS submission here.

I don't think I can tick "Automated tests" yet. I found the script "test_examples.py" and if I executed it, nothing happens. I expect that this means there are no errors. However, it would be great to have some output if everything is alright, e.g. using unittest could help with that.
And another minor remark, it would be great to have a (sub)section in the README that instructs the user about how the tests should be executed, and in the ideal case also describes what is covered by the tests, because for me it looks like only one function per subpackage is covered.

Best regards
Verena

Hi,
in a few places the description of H is hope size instead of hop size.

Best regards
Verena

Hi again,
while reviewing I noticed a few minor things in the README/documentation:

• I don't see a statement of need in the documentation. There is a detailed statement of need in the paper but the review check list also asks for a statement of need in the documentation.
• The info can be found from setup.py but for me as a user it would be helpful to see at one glance which Python version is required (e.g. in the README in section "Installing")
• Community Guidelines: There is a section regarding contributing, but it does not mention how to "Report issues or problems with the software" or "Seek support", although I would expect it through the same channels (mail to the main author, github issue).

Best regards
Verena

Hi, wondering if its possible to update the pandas requirement to allow versions >= 2.0 that was released earlier this year? We are currently having an issue of incompatibility between libfmp and packages that require pandas 2.0 or higher.

https://pandas.pydata.org/docs/dev/whatsnew/v2.0.0.html

Hey there!

I just stumbled over following line from c1/c1s2_symbolic_rep.py, where midi velocity in bytes is normalized to $[0,1]$:

velocity = note.velocity / 128.

Shouldn't it be vel/127.0, because midi velocity is in $[0,127]$, so that 0 maps to 0 and 127 maps to 1.0?

Quick link to the line of code I am referring to:
https://github.com/meinardmueller/libfmp/blob/424127f2cd8317f796ab1591f7c0ec408208e782/libfmp/c1/c1s2_symbolic_rep.py#L70C44-L70C44

Just let me know if I'm thinking wrong or why 128 may be the better choice here! :-)
Anyway, thank you very much for your great work in providing all these reference implementations!!!! 🎉

The numba library version in the requirements (>=0.51.0) doesn't seem to be compatible with Python 3.6.

Would it be better to update the compatible python versions?

the release on pypi still has frozen dependencies which break many environments. Creating a new release could potentially solves these issues....

openjournals/joss-reviews#3434

This package depends on the old naming of pysoundfile, the package was renamed at version 0.8 to just soundfile. Having the old package name as a dependency for this repo means that when other packages have the newer name both are installed, this can cause problems as both packages are imported using import soundfile.

I suggest changing the dependency to soundfile rather than pysoundfile, there is no need to adjust versions as 0.8.x and 0.9.x are available on pypi for both pysoundfile and soundfile, though pysoundfile seems to be no longer available for 0.10.x and above.

I created a fork and ran the tests, with no problem and also tested the save_audio function which worked fine:

https://github.com/pkendrick/libfmp