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

Add load and save for midi files #140

Merged
merged 6 commits into from
Apr 24, 2021
Merged

Conversation

VasanthManiVasi
Copy link
Member

@VasanthManiVasi VasanthManiVasi commented Apr 22, 2021

Hi, I've added the load and save functions to use with FileIO.jl.

Until a PR to FileIO.jl, the following should be added to FileIO/src/registry.jl or can be executed in the REPL for the load/save on midi to work
add_format(format"MIDI", UInt8[0x4d, 0x54, 0x68, 0x64], [".mid", ".midi", ".MID"], [:MIDI => UUID("f57c4921-e30c-5f49-b073-3f2f2ada663e")])

I've tested them and the test sets have passed along with some other tests on midi files locally.

@VasanthManiVasi
Copy link
Member Author

I'm not sure if it was a good idea to have removed readMIDIFile and writeMIDIFile completely.. I just realized that after pushing.. could you please share your thoughts on this? @Datseris

Should those functions stay along with load/save?

@Datseris
Copy link
Member

Datseris commented Apr 22, 2021

Hi,

my understanding is that you have to first do a PR at FileIO, and once a version with your changes is tagged at file IO then we can merge this PR here...? Not sure which one has to be done first. Can you open a PR at FileIO as well, so that the devs there can guide us?

The functions readMIDIFile will need to be deprecated as @deprecate readMIDIFile FileIO.load so that the old names are still available.

@VasanthManiVasi
Copy link
Member Author

VasanthManiVasi commented Apr 22, 2021

Okay, I will add the functions back and deprecate them.

I've opened a PR at FileIO.jl. JuliaIO/FileIO.jl/pull/323

@johnnychen94
Copy link

johnnychen94 commented Apr 22, 2021

JuliaIO/FileIO.jl#323 looks good to me. You can commit a Manifest.toml with that FileIO branch so that the CI here passes, and then make a new MIDI release, update JuliaIO/FileIO.jl#323 with tests, and then we'll merge it and tag a new FileIO release.

I don't know much of the MIDI internal here so it would be great to at least see a green check in CI (by committing a Manifes.toml). After the approval of this PR, you can then remove the Mainfest.toml.

@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2021

Codecov Report

Merging #140 (1b1c203) into master (730ef3a) will decrease coverage by 2.63%.
The diff coverage is 79.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
- Coverage   60.28%   57.65%   -2.64%     
==========================================
  Files           8        8              
  Lines         564      588      +24     
==========================================
- Hits          340      339       -1     
- Misses        224      249      +25     
Impacted Files Coverage Δ
src/midifile.jl 47.94% <79.31%> (-10.26%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 730ef3a...1b1c203. Read the comment docs.

Copy link

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

It's posted via my phone so sorry for not able to format the codes in the comments.

Except two minor comments, other changes are good to me.

@@ -1,5 +1,6 @@
using MIDI
using Test
using FileIO

Choose a reason for hiding this comment

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

Depredations should be explicitly tested as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done (in another PR).

src/midifile.jl Outdated
@@ -111,6 +168,9 @@ function writeMIDIFile(filename::AbstractString, notes::Notes)
return midi
end

@deprecate writeMIDIFile(filename::AbstractString, data::MIDIFile) FileIO.save(filename, data)
Copy link

@johnnychen94 johnnychen94 Apr 24, 2021

Choose a reason for hiding this comment

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

It's more controllable to pass the depredation to 'MIDI.save' instead of 'FileIO.save'. same to 'load'.

(It's FileIO that depends on this package and not this package depends on FileIO.)

Choose a reason for hiding this comment

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

Sorry for the inacuraccy:

Suggested change
@deprecate writeMIDIFile(filename::AbstractString, data::MIDIFile) FileIO.save(filename, data)
@deprecate writeMIDIFile(filename::AbstractString, data::MIDIFile) save(filename, data)

and similar to load

Copy link
Member

Choose a reason for hiding this comment

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

(It's FileIO that depends on this package and not this package depends on FileIO.)

This is so confusing. How does FileIO depend on MIDI? Doesn't MIDI.jl just extend the FileIO.save method?

Copy link

@johnnychen94 johnnychen94 Apr 24, 2021

Choose a reason for hiding this comment

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

MIDI creates a new function save/load, which is not FileIO.save/FileIO.load. MIDI only tags the MIDI.load/MIDI.save function with FileIO.File type, so that FileIO.load/FileIO.save calls the MIDI.load/MIDI.save via invokelatest. That's why I said "FileIO depends on MIDI".

To be clear, MIDI.load and FileIO.load are two functions; they're not specifications of the same function FileIO.load.

Ref: https://juliaio.github.io/FileIO.jl/stable/implementing/

Copy link
Member Author

@VasanthManiVasi VasanthManiVasi Apr 24, 2021

Choose a reason for hiding this comment

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

@johnnychen94 MIDI.load and MIDI.save functions takeFile{format"MIDI"} type in its argument and passing a filename string to them gives a method error. FileIO.load and FileIO.save take a regular string as input and it works fine with them. Would it be fine to use FileIO.load or FileIO.save itself in this case instead of the MIDI functions?

@deprecate writeMIDIFile(filename::AbstractString, data::MIDIFile) FileIO.save(filename, data)

Choose a reason for hiding this comment

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

You can instead use:

@deprecate writeMIDIFile(filename::AbstractString, data::MIDIFile) save(File{format"MIDI"}(filename), data)

The problem with calling FileIO.save is that FileIO.save has a try-and-call mechanism to loop the backends, currently, there might only be MIDI that supports this file format, but what if there's another package, say MIDI2, which happens to have higher priority than MIDI in FileIO? The answer is, when the user calls MIDI.writeMIDIFile, he is eventually calling MIDI2.save, which is definitely not what we want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Done (in another PR).

@Datseris
Copy link
Member

@johnnychen94 your suggestion is a bit weird. You want us to release a version of MIDI that explicitly includes some FileIO branch? And then when installed by users their FileIO installation will also become some branch? Why? Tests passed perfectly before, can't we just tag a normal FileIO release that includes these magic numbers and midi extension?

And you said MIDI should be tested at FileIO? I checked at FileIO's test folder, and the 100s of packages that extend it are not tested there. So where would MIDI be tested?

@johnnychen94
Copy link

You want us to release a version of MIDI that explicitly includes some FileIO branch?

After you approve this PR, the temporary Manifest.toml will be deleted and then you can merge, and tag a release for it.

And you said MIDI should be tested at FileIO?

Yes, there're hundreds of file formats not properly tested inside FileIO. But if you want to make sure FileIO's development doesn't break MIDI's load/save, it's suggested to do so. See JuliaIO/FileIO.jl#319 for an example.

@Datseris
Copy link
Member

Okay, I understand, so you don't need us to tag a release with this Manifest.toml. Just to merge this PR?

@johnnychen94
Copy link

Yes, merge this PR when Manifest.toml is deleted, tag a release, then FileIO will merge JuliaIO/FileIO.jl#323 and tag a new release. After that MIDI CI will get back to green check.

@Datseris
Copy link
Member

Datseris commented Apr 24, 2021

@VasanthManiVasi this code:

    mthd = join(map(Char, read!(f, Array{UInt8}(undef, 4))))
    if mthd != MTHD
        error("Not a valid MIDI file. Expected first 4 bytes to spell 'MThd', got $(mthd)")
    end

does not exist in the new load function you wrote. Why?


@johnnychen94 can you check one more time the file io.jl please? Because the functions save, load are not extended from FileIO.jl, but are just written here as-is.

I've deleted the Manifest.toml file (of course now tests are failing).

@VasanthManiVasi
Copy link
Member Author

@Datseris I've listed MThd as the magic numbers for MIDI files. FileIO uses that to check for MIDI files automatically so we don't have to implement the check again. FileIO also has a utility function to skip those magic numbers when loading the file and I've used that in our load function

open(f) do s
    skipmagic(s) # utility function for skipping magic numbers
    midifile = load(s)
end

@johnnychen94
Copy link

Because the functions save, load are not extended from FileIO.jl, but are just written here as-is.

This is expected, FileIO.save will search and call either MIDI.fileio_save or MIDI.save.

Reference: https://juliaio.github.io/FileIO.jl/stable/implementing/

@Datseris Datseris merged commit ad9c9c5 into JuliaMusic:master Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants