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

Added IPython .show('osmd') using OpenSheetMusicDisplay #326

Open
wants to merge 40 commits into
base: master
from

Conversation

@supersational
Copy link

supersational commented Aug 29, 2018

A proper version of the hack described here: #306

Within an IPython notebook you can now use score.show('osmd') and have the score displayed inline.

Issues:

  • Empty scores sometimes cause a Javascript error
  • Might want to generate a .html file if not in an IPython environment
  • conditional import of IPython needs adding/testing
  • Minor issue but if several scores are shown at the same time it might add an unnecessary amount of script tags in a JS race condition
Sven added 4 commits Aug 29, 2018
@supersational

This comment has been minimized.

Copy link
Author

supersational commented Aug 29, 2018

Actually there's an issue with the script path that needs fixing..
Now fixed in latest commit.

offline = write script file into document
online = link to CDN
@mscuthbert

This comment has been minimized.

Copy link
Member

mscuthbert commented Aug 29, 2018

IPython is not a music21 dependency so be sure that the script runs without it and have as many tests as possible that don’t require it so that other users can test without it. That means all IPython imports should be tested and return a nice error (“install IPython via...”) if a necessary call is made in an environment without it

@mscuthbert

This comment has been minimized.

Copy link
Member

mscuthbert commented Aug 29, 2018

Can more of the logic go into an osmd.py file and out of subconverters? Trying to put that as the bare minimum to the interface. The IPython muse score etc are a bad example to follow.

@supersational

This comment has been minimized.

Copy link
Author

supersational commented Aug 29, 2018

Now moved to seperate module, seemed appropriate since we have additional .js files.

I'm not sure what the convention for import errors are, but it should now ignore missing IPython until .show('osmd') is called.

@supersational

This comment has been minimized.

Copy link
Author

supersational commented Aug 29, 2018

Testing is quite difficult given that it needs to run in a Jupyter environment for it to work. Unfortunately we can't test for javascript console errors, but I'll try adding a notebook for testing in the meantime.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 30, 2018

Coverage Status

Coverage decreased (-0.08%) to 89.943% when pulling 61e3d88 on supersational:opensheetmusicdisplay into 770c1c7 on cuthbertLab:master.

@mscuthbert

This comment has been minimized.

Copy link
Member

mscuthbert commented Aug 30, 2018

Search for the scipy import to see how to test for installation without importing. Importing IPython takes time so it shouldn’t be done unless it’ll be used. The graph module also has a number of nice tricks for imports.

@mscuthbert

This comment has been minimized.

Copy link
Member

mscuthbert commented Aug 30, 2018

I'm concerned that coverage decreased after the tests. We can add IPython to the testing environment if that's necessary to make tests involving IPython, but I wonder if we can't test at least that the code to generate the <script> tags is working properly.

Copy link
Member

mscuthbert left a comment

Thanks for this extensive and amazing pull request. Because it's a substantitive change to the system and I'll need to support it for at least five or more years, I need to scrutinize it quite a bit. Be sure that test/testLint.py passes for the modules as well as Javascript lint testing and that the overall test coverage increases or remains the same when running (testCoverage.py)

music21/__init__.py Outdated Show resolved Hide resolved
music21/common/formats.py Show resolved Hide resolved
def musicXMLToScript(xml, div_id, offline=True):

# print('xml length:', len(xml))
script_dir = os.path.join(os.path.dirname(__file__), 'opensheetmusicdisplay.0.3.1.min.js')

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Aug 31, 2018

Member

(putting the review here, since I can't comment on the file itself): Can the .js file be called from a CDN instead of adding it to music21? The entire music21 noCorpus library is only 4.9MB, so the opensheetmusicdisplay.0.3.1.min.js file adds 25% to the size of all of music21. The noCorpus version of the library is used in some Arduino/Raspberry-PI projects where space is at a premium. We've removed corpus and documentation from those files because of this concern.

One solution if you want to generally serve from local would be to download the file on first call and save to a temp file in the music21 directory, and only redownload if the file can't be found.

Is pinning to a particular version a good idea or is there a .latest we should use?


from music21.opensheetmusicdisplay.osmd import ConverterOpenSheetMusicDisplay

from music21.converter import registerSubconverter

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Aug 31, 2018

Member

Can you put a note in subconverter.py that this is where the OSMD converter is?

Is this file/directory necessary, or could everything be in a single osmd.py file? (see below)

music21/opensheetmusicdisplay/osmd.py Outdated Show resolved Hide resolved

s = corpus.parse('luca/gloria').measures(1, 19)
# s.show('osmd')
ConverterOpenSheetMusicDisplay().show(s, None)

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Aug 31, 2018

Member

this should be in TestExternal. Test() only should be run if there are no side effects. TestExternal is for anything that opens up a browser, plays music, etc. Since the general test system runs all Test() functions, it isn't needed.

This comment has been minimized.

Copy link
@supersational

supersational Oct 15, 2018

Author

Should be fixed now: 9077909. Would be great to be able to check the output (e.g. a notebook that displays all the wigets and .js options?).

music21/opensheetmusicdisplay/osmd.py Outdated Show resolved Hide resolved
music21/opensheetmusicdisplay/osmd.py Outdated Show resolved Hide resolved
music21/opensheetmusicdisplay/osmd.py Outdated Show resolved Hide resolved
# print("firstInstrumentObject.instrumentName",firstInstrumentObject.instrumentName)
assert firstInstrumentObject.instrumentName != None
assert firstInstrumentObject.instrumentName != ''

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Aug 31, 2018

Member

Take a look at the coveralls report to see what is not run and what is not tested in the module.

This comment has been minimized.

Copy link
@supersational

supersational Oct 15, 2018

Author

e9484ae fixed the other issues but as far as I can tell the coveralls doesn't run the testOpenSheetMusicDisplayRuns function? Hopefully the next report will run it.

Sven added 4 commits Oct 15, 2018
offline will download the script to local file on first use, then use
the file for subsequent use
Sven
@supersational

This comment has been minimized.

Copy link
Author

supersational commented Oct 15, 2018

Added some more fixes, still need to sort the JavaScript-as-a-seperate-file issue since I agree it's currently a mess and doing linting would be very helpful.

I think now that we have multiple files involved it should stay as a separate module so we can put associated files in the music/osmd/ e.g. Javascript downloads. If you'd stil like __init__.py to be removed then that's also fine, I was trying to follow the example of other modules when I added it.

Some (still) broken things:

  • missing part-name (as discussed above)
  • coveralls report (will see as the results come through)
  • some of the corpus pieces don't actually convert to musicXML so won't work! may investigate further but it has something to do with minimum note duration.

Again thanks for the detailed comments on this! I'm still getting used to python best practices about testing and so forth, and this project has some excellent standards in place.

Should be useable in notebook currently via:

import music21
s = music21.converter.parse("tinyNotation: 3/4 E4 r f# g=lastG trip{b-8 a g} c4~ c")
s.show('osmd')
@mscuthbert

This comment has been minimized.

Copy link
Member

mscuthbert commented Oct 15, 2018

https://travis-ci.org/cuthbertLab/music21/jobs/441779288 -- ModuleNotFoundError doesn't exist in Py3.5.

Sven
@supersational

This comment has been minimized.

Copy link
Author

supersational commented Oct 15, 2018

Phew seems to have passed finally, hope I didn't use too many of your Travis credits.
For the coverage report would it be necessary to add IPython as an install step to .travis.yaml for the extra test functions to run? Seems like scipy and numpy are already there..

@mscuthbert

This comment has been minimized.

Copy link
Member

mscuthbert commented Oct 15, 2018

Absolutely -- and don't worry about Travis credits. :-)

Copy link
Member

mscuthbert left a comment

Some changes here to pass test/testLint.py -- and be sure to update to latest version of master and fix conflicts in .travis.yml.

registerShowFormats = ('osmd',)
# when updating the script tag osmd will only reload in a notebook if you: Kernel -> restart and clear output, then
# save the notebook and refresh the page. Otherwise the script will stay on the page and not reload.
script_url = """https://github.com/opensheetmusicdisplay/opensheetmusicdisplay/releases/download/0.6.3/opensheetmusicdisplay.min.js"""

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Nov 29, 2018

Member

Have you run test/testLint.py on the your repository? this line looks too long for music21 style.

This comment has been minimized.

Copy link
@supersational

supersational Nov 29, 2018

Author

@mscuthbert just tried that but getting only the 'Usage: testLint.py' message. Is this the right command? $ python music21/test/testLint.py ./music21/osmd/osmd.py tried in git-bash.

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Nov 29, 2018

Member

this should be working -- AH! for some reason, it assumes you're in the music21 directory or fails -- cd to music21 and run 'test/testLint.py osmd/osmd.py' instead and see if that works. Will push a fix

This comment has been minimized.

Copy link
@supersational

supersational Nov 29, 2018

Author

Strange tried python test/testLint.py osmd/osmd.py and getting the same usage info with no error.

--long-help doesn't seem to work either, I get testLint.py: error: unrecognized arguments: --long-help

osmd_file = os.path.join(os.path.dirname(__file__), 'opensheetmusicdisplay.0.6.3.min.js')

def __init__(self):
self.display, self.HTML, self.Javascript = getExtendedModules()

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Nov 29, 2018

Member

split into three lines if possible -- people search for the definition of, say self.HTML by searching for self.HTML =


@staticmethod
def getUniqueDivId():
return "OSMD-div-"+ \

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Nov 29, 2018

Member

linting -- spaces around + and music21 does not use line continuation characters. Wrap inside parentheses please.

def musicXMLToScript(self, xml, divId, offline=False):

# script that will replace div contents with OSMD display
script = open('./music21/osmd/notebookOSMDLoader.js', 'r').read() \

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Nov 29, 2018

Member

as above w/ line continuation characters and spacing. Better to make into multiple statements.


@staticmethod
def addDefaultPartName(score):
# If no partName is present in the first instrument, OSMD will display the ugly 'partId'

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Nov 29, 2018

Member

needs docs and doctests

This comment has been minimized.

Copy link
@supersational

supersational Nov 29, 2018

Author

Hi @mscuthbert I'm afraid this module is running into a lot of problems with tests!

When I try to .show('osmd') in a test or doctest I get: music21.base.Music21ObjectException: cannot support showing in this format yet: osmd

Despite that this works in regular python and in notebooks.

Any ideas?

allInstruments[0].instrumentName = 'Default'
score.coreElementsChanged()

return score

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Nov 29, 2018

Member

this is javascript style to return the element passed in. for music21/python, return None is more appropriate.

# print("adding default inst")
defaultInstrument = Piano()
defaultInstrument.instrumentName = 'Default'
score.insert(None, defaultInstrument)

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Nov 29, 2018

Member

insert at 0.0 not None

# If no partName is present in the first instrument, OSMD will display the ugly 'partId'
allInstruments = list(score.getInstruments(returnDefault=False, recurse=True))

if len(allInstruments)==0:

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Nov 29, 2018

Member

if not allInstruments will pass linter.

defaultInstrument = Piano()
defaultInstrument.instrumentName = 'Default'
score.insert(None, defaultInstrument)
score.coreElementsChanged()

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Nov 29, 2018

Member

coreElementsChanged is only needed if using a .core function. Not needed here or later

from music21.osmd.osmd import ConverterOpenSheetMusicDisplay

from music21.converter import registerSubconverter
registerSubconverter(ConverterOpenSheetMusicDisplay)

This comment has been minimized.

Copy link
@mscuthbert

mscuthbert Nov 29, 2018

Member

registerSubconverter is generally only used for subconverters external to music21 -- I think this can go in converter/subconverters with the other subconverters. Otherwise it will be cleared when external subconverters are cleared.

This comment has been minimized.

Copy link
@supersational

supersational Nov 29, 2018

Author

Ok, there seems to be a lot of logic in each subConverters.py class, should this be a simple cut and paste or more complicated?

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Nov 29, 2018

A new inspection was created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.