search

JuliaDSP / MFCC.jl

Mel Frequency Cepstral Coefficients calculation for Julia
BSD 2-Clause "Simplified" License
33 stars 18 forks source link

Add compat bounds for HDF5 requirement #25

Closed maetshju closed 1 year ago

maetshju commented 3 years ago

This PR adds a compat bound for the HDF5 requirement to be any version older than the v0.15 releases. There were two small changes in v0.14 and up for HDF5 that broke some of the functionality in the io.jl file. Specifically:

In v0.14, the old code used in io.jl throws a deprecation warning, though it errors in v0.15.

The update to the io.jl code is trivial to be compatible with the changes in newer releases of HDF5. However, older versions of Julia (like those used in the CI testing) are not compatible with the newer versions of HDF5. I'm not sure what balance is desired here, so I opted for allowing up to and including the newest version of HDF5 that would still allow the package to work on older versions of Julia.

maetshju commented 3 years ago

I suppose this situation is a bit muddier than I originally thought. The HDF5 changes won't cause MFCC.jl to break on newer version of Julia per se, but newer installs of Julia would probably have newer versions of HDF5. So this change would help prevent issues that might crop up that way if it's desirable for MFCC.jl to continue to work with older versions of Julia.

rob-luke commented 3 years ago

@maetshju could you please rebase from master so the CI tests will run on this PR, thanks.

rob-luke commented 3 years ago

Regarding the versioning issue. Could this be done in two steps. First, update as you suggest with an older version of HDF5 which supports many versions of Julia, then cut a new release. Second, in a follow up PR update HDF5 to the latest version and drop support for older Julia versions and cut a new release.

I suggest our final goal is to support the latest HDF5 version. Maintaining support for an old version would likely require more programmers than we have supporting this organisation. And requiring an old HDF5 version is likely to cause conflicts for end users. But this is just one opinion, let's see what the others suggest.

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 1276331022

Files with Coverage Reduction New Missed Lines %
src/mfccs.jl 7 51.79%
src/rasta.jl 16 36.15%
src/feacalc.jl 19 27.73%
<!-- Total: 42 -->
Totals Coverage Status
Change from base Build 1276266728: 3.9%
Covered Lines: 203
Relevant Lines: 470
💛 - Coveralls
coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 1276331022

Totals Coverage Status
Change from base Build 1276266728: 2.9%
Covered Lines: 176
Relevant Lines: 417
💛 - Coveralls
maetshju commented 3 years ago

Git for Windows has been giving me trouble with this particular PR since I started working on it, but I believe that I have successfully done the rebase. It looks like the CI has been triggered, at least.

I personally like the idea of making two releases to fix the issue. There are still some accepted PRs that have yet to make it into a release, like https://github.com/JuliaDSP/MFCC.jl/pull/19, which the releases would fix as well.

maetshju commented 2 years ago

Is there still an interest in cutting a version based on this old HDF5 requirement and then cutting another one that updates the io.jl file to use new HDF5 conventions? It is getting difficult to have new code rely on this package because of HDF5. From what I recall about the last set of tests that were run, the failures were due to there not being any HDF5 binaries available via HDF5.jl for x86 Ubuntu.

I have a different branch I have worked on that I think has resolved the HDF5 issues and could make a new PR for cutting a new version. It passes the unit tests on my local machine (Julia 1.8.2, Windows 11) and the CI for newer Julia versions in a separate branch I made to run the GitHub CI action.

davidavdav commented 1 year ago

Hello, Thanks for this PR, I think I fixed it myself independently, today. And sorry for not being very responsive on the matter, somehow I wasn't aware of the PR.

maetshju commented 1 year ago

Thank you for doing that! I have been able to install and import the new version of this package with a newer environment, so it looks like the your update worked just fine. And no worries about responsiveness; it all worked out in the end.