Work on noteworthy 1 75 binary reader #1247

nvuaille · 2022-03-06T18:52:41Z

Improve support for many nwc objects:
- alterations
- grace notes
- tempo mark
- triplets
- text position
- chords / multiple pitches
- repeat brackets
Readibility:
- put constants in their own file
- rename some methods (e.g. NWCStaff.parse() -> NWCStaff.parseStaff())
Use exception to print error infos on music21 objects creation
Update documentation
Update tests

refeers to #1123

coveralls · 2022-07-06T12:53:09Z

Coverage: 93.081% (+0.03%) from 93.053% when pulling 1e8882d on nvuaille:Noteworthy into b474fd4 on cuthbertLab:master.

jacobtylerwalls

Thank you for working on this improved converter! The adjusted doctest showing the better results is great.

Let me know if you have any questions or if I misunderstood something (since I don't know this format).

music21/noteworthy/translate.py

music21/noteworthy/binaryTranslate.py

music21/noteworthy/translate.py

nvuaille · 2022-07-09T15:01:23Z

@jacobtylerwalls , thanks for your review, here is a new version improved with your comments

jacobtylerwalls · 2022-07-10T01:08:38Z

Great, thanks for the changes. I expect Myke will want to weigh in with his review. At that point please avoid force-pushing, as whoever merges has the option to squash the commits, and in the meantime it prevents people from assessing what changed between reviews. Thanks!

mscuthbert

I still need to review from l. 558 of nw/translate onwards, but looking very good. Some changes requested, but nothing fundamental, and this will move to acceptance as soon as they're made and I'm able to verify that my collections of nwc and nwctxt files still parse.

music21/noteworthy/binaryTranslate.py

mscuthbert · 2022-07-12T20:33:02Z

music21/noteworthy/binaryTranslate.py

+        instruName = self.instrumentName if self.instrumentName else 'NoName'
+        label = self.label.decode('utf-8') if self.label else instruName
+
+        staffString = '|AddStaff|Name:' + label
+        dumpObjects.append(staffString)
+
+        staffInstruString = '|StaffInstrument|Name:' + instruName
+        if instruName in constants.MidiInstruments:
+            staffInstruString += '|Patch:'
+            staffInstruString += str(constants.MidiInstruments.index(self.instrumentName))
+
+        staffInstruString += '|Trans:' + str(self.transposition)


move all added sections to separate method and test this.

'NoName' doesn't seem right for this -- must it have a name or are these lines able to be omitted?

Indeed, I think falling back to the first midi instru ( Acoustic Grand Piano) is a better choice.

Also, I'm not sure to see what do you mean by "all added sections" ?

music21/noteworthy/binaryTranslate.py

mscuthbert · 2022-07-13T06:04:56Z

music21/noteworthy/translate.py

+        self.getCurrentVoice().append(n)
+
+    # current voice has shorter duration. If no voices, return current measure
+    def getCurrentVoice(self):


needs docs, and return typing.

Especially since it doesn't necessarily return a stream.Voice object as the name implies.

as this is now called for every note, it needs to be optimized as much as possible. I'm seeing most optimized version as:

if self.currentMeasure.isFlat: # O(1) in most cases. return self.currentMeasure # O(n) only if voices already exist. return min( self.currentMeasure.getElementsByClass(stream.Voice), key=lambda v: v.quarterLength )

Far better, thanks for the suggestion!

music21/noteworthy/translate.py

mscuthbert · 2022-07-13T06:08:29Z

music21/noteworthy/translate.py

+    # current voice has shorter duration. If no voices, return current measure
+    def getCurrentVoice(self):
+        voice = None
+        shorter = 999


this should be a float so that typing works for it. And typed as OffsetQL since it can also be a Fraction. float('inf') would be the best way of implying that any voice is shorter than this.

and rename as shortestQL

Resolved by using your implementation

mscuthbert · 2022-07-13T06:09:43Z

music21/noteworthy/translate.py

+        for item in self.currentMeasure:
+            if isinstance(item, stream.Voice):


iterate as for v in self.currentMeasure.getElementsByClass(stream.Voice):

that way you're not setting activeSite etc. for non-matching elements.

Resolved by using your implementation

music21/noteworthy/translate.py

nvuaille · 2022-07-13T08:53:55Z

Hi @mscuthbert , thanks for the review!

I'm a bit busy in july but I should come back on this during august.

jacobtylerwalls

FYI still seeing fromDisplayPitch (nonexistent method) called in latest commit, so unresolving the relevant comment.

* Improve support for many nwc objects: * alterations * grace notes * tempo mark * triplets * text position * chords / multiple pitches * repeat brackets * Readibility: * put constants in their own file * Use exception to print error infos on music21 objects creation * Update documentation * Update tests

nvuaille

I just came back on it, sorry for the delay. I rebased on master but I created a new commit for the changes related to the review

music21/noteworthy/binaryTranslate.py

nvuaille · 2022-11-04T15:33:01Z

music21/noteworthy/binaryTranslate.py

-    def timeSig(self):
+    def timeSignature(self):


No reason indeed. I'll keep the timeSig as it is consistent with the nwctxt keyword used below.

nvuaille · 2022-11-04T15:47:10Z

music21/noteworthy/translate.py

+        self.getCurrentVoice().append(n)
+
+    # current voice has shorter duration. If no voices, return current measure
+    def getCurrentVoice(self):


Far better, thanks for the suggestion!

nvuaille · 2022-11-04T15:51:01Z

music21/noteworthy/translate.py

+        for item in self.currentMeasure:
+            if isinstance(item, stream.Voice):


Resolved by using your implementation

nvuaille · 2022-11-04T15:51:06Z

music21/noteworthy/translate.py

+    # current voice has shorter duration. If no voices, return current measure
+    def getCurrentVoice(self):
+        voice = None
+        shorter = 999


Resolved by using your implementation

nvuaille · 2022-11-04T16:04:56Z

music21/noteworthy/binaryTranslate.py

+        instruName = self.instrumentName if self.instrumentName else 'NoName'
+        label = self.label.decode('utf-8') if self.label else instruName
+
+        staffString = '|AddStaff|Name:' + label
+        dumpObjects.append(staffString)
+
+        staffInstruString = '|StaffInstrument|Name:' + instruName
+        if instruName in constants.MidiInstruments:
+            staffInstruString += '|Patch:'
+            staffInstruString += str(constants.MidiInstruments.index(self.instrumentName))
+
+        staffInstruString += '|Trans:' + str(self.transposition)


Also, I'm not sure to see what do you mean by "all added sections" ?

mscuthbert · 2023-07-01T09:03:08Z

Sorry that I forgot to merge this. Looks like it's good to go

Changes made over force push

nvuaille · 2023-07-02T20:37:52Z

Obviously I also forgot this one ... Thanks a lot for the integration !

jacobtylerwalls linked an issue Mar 6, 2022 that may be closed by this pull request

Improving Noteworthy converter support #1123

Closed

nvuaille force-pushed the Noteworthy branch from 291161e to 4e43159 Compare June 14, 2022 12:37

jacobtylerwalls reviewed Jul 6, 2022

View reviewed changes

nvuaille force-pushed the Noteworthy branch from 4e43159 to a057e51 Compare July 9, 2022 15:00

nvuaille force-pushed the Noteworthy branch from a057e51 to f89e526 Compare July 9, 2022 15:16

jacobtylerwalls approved these changes Jul 10, 2022

View reviewed changes

mscuthbert requested changes Jul 13, 2022

View reviewed changes

jacobtylerwalls previously requested changes Jul 30, 2022

View reviewed changes

mscuthbert closed this Aug 10, 2022

mscuthbert reopened this Aug 10, 2022

nvuaille added 2 commits November 4, 2022 16:07

First round of reviews

726caac

nvuaille commented Nov 4, 2022

View reviewed changes

nvuaille force-pushed the Noteworthy branch from 15ae3a6 to 726caac Compare November 4, 2022 16:06

nvuaille added 2 commits November 6, 2022 16:59

Fixes for CI

67c12ed

Coverage: add a 1.75 file for testing

0203a2f

jacobtylerwalls closed this Dec 10, 2022

jacobtylerwalls reopened this Dec 10, 2022

mscuthbert closed this Jan 30, 2023

mscuthbert reopened this Jan 30, 2023

mscuthbert added 2 commits January 31, 2023 16:21

lint

98705d5

flake: training whitespace

1e8882d

mscuthbert approved these changes Jul 1, 2023

View reviewed changes

mscuthbert merged commit 8be7736 into cuthbertLab:master Jul 1, 2023

nvuaille deleted the Noteworthy branch July 2, 2023 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Work on noteworthy 1 75 binary reader #1247

Work on noteworthy 1 75 binary reader #1247

nvuaille commented Mar 6, 2022

coveralls commented Jul 6, 2022 •

edited

Loading

jacobtylerwalls left a comment

nvuaille commented Jul 9, 2022

jacobtylerwalls commented Jul 10, 2022

mscuthbert left a comment

mscuthbert Jul 12, 2022

nvuaille Nov 4, 2022

nvuaille Nov 4, 2022

mscuthbert Jul 13, 2022

mscuthbert Jul 13, 2022

mscuthbert Jul 13, 2022

nvuaille Nov 4, 2022

mscuthbert Jul 13, 2022

mscuthbert Jul 13, 2022

nvuaille Nov 4, 2022

mscuthbert Jul 13, 2022

nvuaille Nov 4, 2022

nvuaille commented Jul 13, 2022

jacobtylerwalls left a comment

nvuaille left a comment

nvuaille Nov 4, 2022

nvuaille Nov 4, 2022

nvuaille Nov 4, 2022

nvuaille Nov 4, 2022

nvuaille Nov 4, 2022

mscuthbert commented Jul 1, 2023

nvuaille commented Jul 2, 2023

		for item in self.currentMeasure:
		if isinstance(item, stream.Voice):

Work on noteworthy 1 75 binary reader #1247

Work on noteworthy 1 75 binary reader #1247

Conversation

nvuaille commented Mar 6, 2022

coveralls commented Jul 6, 2022 • edited Loading

jacobtylerwalls left a comment

Choose a reason for hiding this comment

nvuaille commented Jul 9, 2022

jacobtylerwalls commented Jul 10, 2022

mscuthbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvuaille commented Jul 13, 2022

jacobtylerwalls left a comment

Choose a reason for hiding this comment

nvuaille left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscuthbert commented Jul 1, 2023

nvuaille commented Jul 2, 2023

coveralls commented Jul 6, 2022 •

edited

Loading