[myank] Add -l flag to select line range and improve cutting null tokens

[WolfgangDrescher]

This PR enables to pass a line range to myank to filter a specific segment of the score and adds improved support for sustained notes that are not tied. See this exmaple:

VHV

Probably this is not the best approach because in combination with myank this feature needs bar numbers to be present in the source kern file, but filtering by a line range should not depend on bar numbers.

Maybe it's better to add a new lyank method to the myank tool? Or would it be better to create a separate command for this? Since myank and lyank need similar features, like the one described in humdrum-tools/verovio-humdrum-viewer#780 and humdrum-tools/verovio-humdrum-viewer#781, I suggest to keep it within myank.

[craigsapp]

I think it is not necessary to have a separate tool/filter called lyank and adding the functionality to myank is good.

[WolfgangDrescher]

What is the best way to modify a token? I am trying something like below but it has no effect. infile[i] will not print the value of the modified token with token->setText(tokenText).

if (startLineHandled == false) {
	HLp line = infile.getLine(i);
	if(line->isData()) {
		vector<HTp> tokens;
		line->getTokens(tokens);
		for (HTp token : tokens) {
			if (token->isDataType("**kern") && token->isNull()) {
				string recip = Convert::durationToRecip(token->getDurationToNoteEnd());
				string pitch = Convert::base40ToKern(token->resolveNull()->getBase40Pitch());
				string tokenText = recip + pitch + "]";
				token->setText(tokenText);
			}
		}
		startLineHandled = true;
	}
}
m_humdrum_text << infile[i] << "\n";

[craigsapp]

The problem is that when a token's content is changed with HumdrumToken::SetText(), the HumdrumLine's text is not updated automatically. HumdrumLine text is used to print HumdrumFiles, so when you print the file, none of the HumdrumToken::setText() changes are shown without calling that function.

The easiest thing to do is to is call HumdrumFile::createLinesFromTokens() just before printing. An additional way is to call HumdrumLine::createLineFromTokens() after changing any token texts on the line.

Ideally I should add a more automated way of doing this: when HumdrumToken::setText is called, this would inform the HumdrumLine that it will need to regenerate its text when printing, and when printing a Humdrum file, the HumdrumLines would regenerate if there are any changes to tokens that have been reported. Or I should get rid of the complicated system of how this is done 😉

[craigsapp]

if(line->isData()) {

And don't forget to put spaces after if keywords 😜

[craigsapp]

Note that HumdrumToken->isDatatType("**kern") has a convenience alias: HumdrumToken->isKern().

[craigsapp]

For the line in the sample code:

string pitch = Convert::base40ToKern(token->resolveNull()->getBase40Pitch());

For the output of HumdrumToken::resolveNull() there may be cases where there is no non-null data token above it in the same track. This may happen in real data if there is a grace note at the start of the score, but other **kern spines with have a null token at the start of the score that do not have a grace note. This may happen just after spine splits where there are grace notes in only some **kern spines, or if there are data errors (which will cause rhythm parsing problems as well). When this happens, the null token will resolve to itself. This means that you are sending a null token to HumdrumToken::getBase40Pitch().

I don't know what getBase40Pitch() returns when given a null token. Ideally perhaps a rest, but that cannot be done properly since in theory getBase40Pitch could return 0 for a very low C#, or even negative values for very lows below the lowest C on a piano keyboard. So I would first check if the resolved null is itself null before trying to extract the pitch.

Also related: if the resolved null token is a rest, I would not continue on to getBase40Pitch().

I double checked and resolveNull() will never return a NULL pointer, so that does not need to be checked (if the HumdrumToken detects a NULL pointer, that means that it needs to tell the Humdrum file to resolve null tokens -- so this analysis is only done if the `HumdrumToken::resolveNull() is called.

Here is more robust code to replace the above line:

HTp resolution = token->resolveNull();
if (resolution->isNull()) {
        continue;
}       

string pitch;
if (resolution->isRest()) {
        pitch = "r";
} else {
        pitch = resolution->getBase40Pitch();
}

Or with the triple operator that you like to use:

HTp resolution = token->resolveNull();
if (resolution->isNull()) {
        continue;
}       
string pitch = resolution->isRest() ? "r" : resolution->getBase40Pitch();

[craigsapp]

There was also the Convert::base40ToKern() that needs to be added in the last post.

Here is some adjustments to the sample that also includes a flag to keep track of if HumLine needs to be updated:

if (startLineHandled == false) {
	HLp line = infile.getLine(i);
	bool lineChange = false;
	if (line->isData()) {
		vector<HTp> tokens;
		line->getTokens(tokens);
		for (HTp token : tokens) {
			if (token->isDataType("**kern") && token->isNull()) {
				HTp resolve = token->resolveNull();
				if (resolve->isNull()) {
					continue;
				}
				string recip = Convert::durationToRecip(token->getDurationToNoteEnd());
				string pitch;
				if (resolve->isRest()) {
					pitch = "r";
				} else {
					pitch = Convert::base40ToKern(resolve->getBase40Pitch());
				}
				string tokenText = recip + pitch + "]";
				token->setText(tokenText);
			}
		}
		startLineHandled = true;
	}
	if (lineChange) {
		line->createLineFromToken();
	}
}
m_humdrum_text << infile[i] << "\n";

[craigsapp]

Here is an improvement for the extraction of a pitch string (avoid going through a numeric conversion):

            string pitch;
            if (hre.search(resolve, "([rRA-Ga-gxyXYn#-]+)") {
               pitch = hre.getMatch(1);
            }

There is also a problem because chords are not yet handled...

This will also handle semi-pitched notes which contain an R, as well as invisible notes which have yy and forced display of accidentals with X (which may or may not be needed/wanted).

[WolfgangDrescher]

I have improved this now and it works okay so far:

Source | VHV

myank -m 4

myank -l 40-50

myank -l 41-50

myank -l 41-45

One thing I would improve is the handling of the bar lines. I think if the section does not end at the end of a bar then the bar line should not be displayed.

Maybe bar numbers should be displayed when using --lines and when the section starts at the beginning of a measure. (See the missing 4 in example myank -l 40-50)

Additionally I could add an option to hide the notes that are cut and now need a tie. Although I'm not sure if this is useful since then there will be missing notes in the harmony (e.g. for fb analysis).

And what you describe in humdrum-tools/verovio-humdrum-viewer#780 (comment) could still be implemented: when notes are longer than the section or when the null resolved notes start before the section they could be displayed with <space> or as hidden notes in the other voices.

Anything I did not consider yet? You mentioned chords and maybe I should have a look at spine splits.

Ideally we should refactor this so the -l option would not depend on bar numbers. But I don't mind about this for now.

[WolfgangDrescher]

And don't forget to put spaces after if keywords 😜

We need a linter 😇️. Clang is not configurable the way you want it?

[WolfgangDrescher]

Note that for some examples it will output !!!COM... multiple times when running -I -l x-y. Probably this should be the missing tandem interpretations for clef, meter, key signature, etc.

!!!COM: Lassus, Orlandus
!!!CDT: 1532-1594/06/14
!!!OPR: Geistliche Psalmen mit dreyen Stimmen
!!!ONM: 5
!!!OTL@@LA: Verba mea auribus
!!!OTL-incipit@@DE: Vernimb Herr meine Wort
**kern	**text	**kern	**text	**kern	**text
*Ivox	*	*Ivox	*	*Ivox	*
*I"Bassus	*	*I"Tenor	*	*I"Cantus	*
*I'B	*	*I'T	*	*I'C	*
!!!COM: Lassus, Orlandus
!!!COM: Lassus, Orlandus
!!!COM: Lassus, Orlandus
!!!COM: Lassus, Orlandus
!!!COM: Lassus, Orlandus
!!!COM: Lassus, Orlandus
2d	re-	2f#	re-	4a	-dach-
.	.	.	.	4dd	-te
2e-X	-den	2.g	-den	8cc	re-
.	.	.	.	8b-	.
.	.	.	.	8b-	.
.	.	.	.	16a	.
.	.	.	.	16g	.
=10	=10	=10	=10	=10	=10
1d	mein/	.	.	2a	.
.	.	8f#i	.	.	.
.	.	8e	.	.	.
.	.	2f#i	.	2a	-den
2r	.	[2g	mein/	2g	mein/
=	=	=	=	=	=
*-	*-	*-	*-	*-	*-
!!!RDF**kern: l = terminal long
!!!RDF**kern: i = editorial accidental
!!!AGN: Psalm; Tricinium
!!!LIB: Ulenberg, Caspar
!!!LIB-CDT: 1548/12/24-1617/02/16
!!!YOR: https://mdz-nbn-resolving.de/details:bsb00075346
!!!YOR-alternative: https://mdz-nbn-resolving.de/details:bsb00072990
!!!YOO: München, Bayerische Staatsbibliothek
!!!URL-scan: https://www.digitale-sammlungen.de/de/view/bsb00075346?page=15 Cantus
!!!URL-scan: https://www.digitale-sammlungen.de/de/view/bsb00075346?page=75 Tenor
!!!URL-scan: https://www.digitale-sammlungen.de/de/view/bsb00075346?page=131 Bassus
!!!IIIF: https://api.digitale-sammlungen.de/iiif/presentation/v2/bsb00075346/manifest
!!!EED: Wolfgang Drescher
!!!END: 2022/12/22
!!!ENC: Wolfgang Drescher
!!!EEV: 2022/12/22
!!!title: @{ONM}. @{OTL} / <i>@{OTL-incipit@@DE}</i>

[craigsapp]

We need a linter 😇️. Clang is not configurable the way you want it?

In theory this would be nice, but I like to vertically align code in cases such as when there is a lot of repetition, and in the *.h files I like to line up the member function names vertical to make it faster to browse through the list of functions. So a linting option for clang-format that does not disturb that would be good to have (but I have not tried to see). I also like to do it manually so that if I see some badly formatted code, it tells me that I have not spent much time on that code (so might be buggy or need some more refinement).

In verovio there is a style file for clang-format:
https://github.com/rism-digital/verovio/blob/develop/.clang-format

And before I submit updates to code in the verovio repository (excluding humlib which is an external library to verovio), I run clang-format -i filename The -i option is for "in place" which means alter the file with the conformant style.

[craigsapp]

For cases such as this one:

myank -l 41-45

There should not be a barline added at the end of the yanked line (there is a 16th duration more before the barline)

Current output:

 myank -l 41-45 /tmp/z.krn  | rid -G

**kern	**text	**kern	**text	**kern	**text
*clefC3	*	*clefC1	*	*clefG2	*
*k[b-]	*	*k[b-]	*	*k[b-]	*
*g:dor	*	*g:dor	*	*g:dor	*
*M2/1	*	*M2/1	*	*M2/1	*
*met(C|)	*	*met(C|)	*	*met(C|)	*
*MM180	*	*MM180	*	*MM180	*
4f]	.	4a]	.	4.cc	dir
[4..e-	kom-	[4..g	dir	.	.
.	.	.	.	8b-	.
.	.	.	.	8b-	.
.	.	.	.	16a	.
=	=	=	=	=	=
*-	*-	*-	*-	*-	*-

Desired output:

**kern	**text	**kern	**text	**kern	**text
*clefC3	*	*clefC1	*	*clefG2	*
*k[b-]	*	*k[b-]	*	*k[b-]	*
*g:dor	*	*g:dor	*	*g:dor	*
*M2/1	*	*M2/1	*	*M2/1	*
*met(C|)	*	*met(C|)	*	*met(C|)	*
*MM180	*	*MM180	*	*MM180	*
4f]	.	4a]	.	4.cc	dir
[4..e-	kom-	[4..g	dir	.	.
.	.	.	.	8b-	.
.	.	.	.	8b-	.
.	.	.	.	16a	.
*-	*-	*-	*-	*-	*-

This will prevent displaying a barline where there should not be one:

[WolfgangDrescher]

There should not be a barline added at the end of the yanked line (there is a 16th duration more before the barline)

This is fixed now. Additionally bar numbers are now displayed when the yanked section (with -l) starts directly at the beginning of a bar.

It might be useful to enhance the -m option to allow similar functionality to -l. For example -l is fragile since adding any lines before the lines to be extracted will mess up the line extraction range.

What do you mean with «adding any lines before the lines to be extracted will mess up»?

For the case myank -l 41-45, it could alternately be described as myank -m 4q1-4q3.75.

I would simply convert those measures into a line range. How about -m 4-4.75 (or -m 4-4.9375 when you want to select the last 16th)? But I see that this can be problematic since it is depending on the time signature. How should we handle when there is no slice for a specific beat?

In addition, another option (-q or -Q) could be in reference to the position of the music in terms of quarter notes since the beginning of the music. In this case myank -q 25-27.75 would be equivalent to the above two examples, starting 25 quarter notes into the score and ending just before quaternote 27.75.

This sound interesting for my analysis ideas of Bach chorales as I want to interate over each n quater notes segments. But how should we handle when there is no slice existing in the score for a selected beat (e.g. rhythm goes in whole notes but you select quater note 2-3).

Either simply display the whole note which is not ideal, or we need a complicated manipulation of this slice.

Or -q and -Q could be used for a different purpose (and the above -q option can be give a different name. When there are grace notes on either side of the quarter-note range, then -q would include the grace notes before the first durational line at quarter note 25, and -Q would include grace notes before the first durational line at quarter note 27.75.

Maybe -g for grace notes? I would add all data lines that occur "directly" before the start of the section and have a duration of 0. This should not be relevant for the end of a selection, correct?

[WolfgangDrescher]

Note that for some examples it will output !!!COM... multiple times when running -I -l x-y. Probably this should be the missing tandem interpretations for clef, meter, key signature, etc.

Actually the problem is not in combination with -I but when the first yanked measure does not start at the beginning of the measure the tandem interpretations will not be output correctly and the !!!COM: Lassus, Orlandus (the first line, I guess) is displayed instead.

This seems to be related to Tool_myank::adjustGlobalInterpretationsStart. Any idea on how to fix this?

	if (clefQ) {
		for (i=0; i<infile[ii].getFieldCount(); i++) {
			ptrack = infile.token(ii, i)->getTrack();
			x  = outmeasures[index].sclef[ptrack].x;
			y  = outmeasures[index].sclef[ptrack].y;
			if ((x>=0)&&(y>=0)) {
				m_humdrum_text << infile.token(x, y);
			} else {
				m_humdrum_text << "*";
			}
			if (i < infile[ii].getFieldCount()-1) {
				m_humdrum_text << "\t";
			}
		}
		m_humdrum_text << "\n";
	}

ptrack is -1. I'm not sure if this is becuase i or ii are already wrong, or if there is another problem.

[WolfgangDrescher]

I found a solution for this in 3fc4f16. But I think it's better if you review this as I don't know well the logic of MeasureInfo in myank.

[WolfgangDrescher]

I added options to hide everything before the yanked data starts (--hide-starting) and to hide everything after the yanked data ends (--hide-ending). The result will always output the starting exclusive interpretation and the terminating spine interpretation.

[WolfgangDrescher]

Is there anything you want me to add or test for this PR? Your suggestions from humdrum-tools/verovio-humdrum-viewer#780 (comment) are not implemented yet where notes that exceed the the myank (beginning or end) are displayed with their full duration.

But currently I don't think this is needed for a initial version of this feature. I can come back to this when someone needs it.

So I think this PR should be ready to merge, or at least ready for review.

[craigsapp]

The PR code looks great. There were just a few very minor things I just commented on. You can tell me when you push changes for those, and then I will integrate the PR and compile it for VHV use.

Another small thing is that sometimes the lines are a bit long. I used to limit to 80 characters on a line, but no longer bother with that. But it would be good to strive to avoid long lines in any case. Sometimes I break long lines onto two lines in this way: after a line break inside of a condition, such as if, I will indent the continuation line(s) by two tab characters (not one, since that is the indentation level of the code inside of the condition block).

[craigsapp]

Also you should document new options on https://doc.verovio.humdrum.org/filter/myank for tools as they are added. But I just looked and see there is no documentation for myank on that website, so you are off the hook for that (at the moment :-)

As a coincidence, I showed someone today how measures can be selected from a full score with myank, and now I see how he could have missed it (and was spending lots of time deleting Humdrum lines manually to do the same thing).

The myank tool was ported from the Humdrum Extras code base, and there is documentation:
http://extras.humdrum.org/man/myank

New options cannot be added to that page, since the old version will not have the new options. At some point I will transfer over the documentation for the humlib version of myank and then you can add documentation for the new options.

[WolfgangDrescher]

The PR code looks great. There were just a few very minor things I just commented on. You can tell me when you push changes for those, and then I will integrate the PR and compile it for VHV use.

Okay, I will have a look again, must have missed some.

At some point I will transfer over the documentation for the humlib version of myank and then you can add documentation for the new options.

You can assign me an issue then.

One thing I noticed: --hide-ending will of course also remove !!!RDF**kern comments (I lost terminal long notes). Do you think those comments should always be included even if --hide-starting or --hide-ending are given? Or we could add an other options for this (--rdf). Or we introduce an option where you can pass reference records that you want to keep: myank --keep-reference-records: COM,OPR,ONM,OTL,RDF.

Alternatively, this could probably be achieved with shed. Maybe this is even a better option than adding edge case options for such things.

[craigsapp]

One thing I noticed: --hide-ending will of course also remove !!!RDF**kern comments (I lost terminal long notes). Do you think those comments should always be included even if --hide-starting or --hide-ending are given? Or we could add an other options for this (--rdf). Or we introduce an option where you can pass reference records that you want to keep: myank --keep-reference-records: COM,OPR,ONM,OTL,RDF.

Yes, I think that reference records should always be preserved. There can be a fine distinction: Any reference records before the first exclusive interpretation and last data termination line *- should always be transferred to the output of myank. As you noted, removing an RDF record can become problematic.

Reference records are "timeless" so they should be unaffected by the time region selected by myank. When reference records are stored inside of the score rather than outside (which is allowed, but typically unusual), then I think it is reasonable to allow myank to throw them away if they are not in the region to be yanked. For example, if you place a reference record inside the score in measure 10, but you myank measures 15–20, then the reference record in measure 10 should not be in the output data.

[craigsapp]

Okay, I will have a look again, must have missed some.

You did not miss them: I just newly added them (I can point them out if you did not get email messages about them from Github).

[WolfgangDrescher]

Sometimes I break long lines onto two lines in this way: after a line break inside of a condition, such as if, I will indent the continuation line(s) by two tab characters (not one, since that is the indentation level of the code inside of the condition block).

JS linters (even php-cs-fixer) will format this completely different. I'm used to something like this:

if (
    getBoolean("lines") &&
    lastDataLine >= 0 &&
    infile.getLine(lastDataLine)->getDurationToBarline() > infile.getLine(lastDataLine)->getDuration()
) {

I run clang-format on the source and checked that the usual styling would be I changed it to something similar in a1eda08. It looks weird to me, but I hope it follows the convention in humlib now.

But personally I prefer having long lines, even though I tend to use verbose variable names (and the editor can still wrap lines if you want it).

You did not miss them: I just newly added them (I can point them out if you did not get email messages about them from Github).

Do you mean this comment: #55 (comment)? If not I did not get a notification from GitHub for other review comments, so I'd be glad If you point them out again.

[WolfgangDrescher]

Another thing I noticed is that myank will always have a trailing new line at the end of the output. This is not a huge problem, but probably we should remove it to output a valid humdrum syntax. However I could not find a way for the stringstream type of m_humdrum_text to remove it.

[craigsapp]

Another thing I noticed is that myank will always have a trailing new line at the end of the output. This is not a huge problem, but probably we should remove it to output a valid humdrum syntax. However I could not find a way for the stringstream type of m_humdrum_text to remove it.

Yes, it would be best to remove it. Was it there before you added code? There is also a problem in the text editor on VHV, where the editor loves to add an extra line at the bottom of the text, which is hard to get rid of.

But personally I prefer having long lines, even though I tend to use verbose variable names (and the editor can still wrap lines if you want it).

Long lines are not a problem, so don't worry about it too much. (but also don't be excessive with them :-)

if (
    getBoolean("lines") &&
    lastDataLine >= 0 &&
    infile.getLine(lastDataLine)->getDurationToBarline() > infile.getLine(lastDataLine)->getDuration()
) {

I would do this:

if (getBoolean("lines") & lastDataLine >= 0 &&
          infile.getLine(lastDataLine)->getDurationToBarline() > infile.getLine(lastDataLine)->getDuration()) {
    i = i + 2;
}

There is a double indent of the continuation of the condition, and the main body of the block has a normal single indentation. Just having if ( seems to be visually distracting to me.

if lastDataLine is guaranteed to be valid (non-negative, and not larger than the number of lines in the file, then I might do something like this:

HLp line = &infile.getLine(lastDataLine);
if (getBoolean("lines") && (lastDataLine >= 0) && (line->getDurationToBarline() > line->getDuration())) {
    i = i + 2;
}

Note the extra () so it is clear there are three conditions being anded together.

---

I will merge this PR now and adjust a few things in style I was mentioning. The main thing is that std:: should not be used in the .cpp files since they (should) always have using namespace std; in them. In the .h files, the opposite should happen: There should be no using namespace std; and std:: should always prefix STL class names.

[craigsapp]

Another style thing is for the triple operator:

int w = x ? y : z;

There should be a space between the y string and the colon. Otherwise it looks like a label for a goto statement, or something else weird.

[craigsapp]

Why do you use the variable name d in this code:

                  for (int d = i; d <= endLineNumber - 1; d++) {
                     if (!infile.getLine(d)->isCommentGlobal()) {
                        nextLineIndexWithSpines = d;
                        break;
                     }
                  }

No problem with that. I customarily use j in such situations where there is a loop already for i. (then k, m (skipping l since it looks like 1, etc.))

[craigsapp]

Regarding long variable names: Having a variable name that is describes what it is being used for is good. I tend to prefer to have a comment describing what the variable does along with a shorter name, unless it is important to keep track of the meaning of the variable.

There is also another way of having both by making an alias reference variable:

    int someLongNameDescribingWhatTheVariableDoes = 42;
    int& myvar = someLongNameDescribingWhatTheVariableDoes;

Now you can use myvar in the code, and it is really the variable someLongNameDescribingWhatTheVariableDoes.

In iohumdrum.cpp I use this often, such as:

void HumdrumInput::processHangingTieStarts()
{              
    std::vector<humaux::StaffStateVariables> &ss = m_staffstates;
    for (int i = 0; i < (int)ss.size(); ++i) {
        for (auto &it : ss[i].tiestarts) {
            processHangingTieStart(it);
        }         
    }          
}

This also reminds me what the type of m_staffstates is. For me this is clearer to read rather than:

void HumdrumInput::processHangingTieStarts()
{              
    std::vector<humaux::StaffStateVariables> &ss = m_staffstates;
    for (int i = 0; i < (int)m_staffstates.size(); ++i) {
        for (auto &it : m_staffstates[i].tiestarts) {
            processHangingTieStart(it);
        }         
    }          
}

In the previous code snippet, I see the flow of the logical structure clearer and the names of the variables do not get in the way (particularly if they are repeating).

[craigsapp]

Regarding std::min and std::max, I see that the std:: is required, perhaps due to <cmath>.

[WolfgangDrescher]

There should be a space between the y string and the colon. Otherwise it looks like a label for a goto statement, or something else weird.

Must have been a typo. Usually I do this.

Why do you use the variable name d in this code:

There was a bug with nested loop where I used two times i. But also because in JavaScript var, const and let are tread different within scopes. I do not have a clue if in C++ there is an equivalent to those JS scopes. But to prevent any issues at some point where i already has an additional value from another loop, I started to go through the alphabet for those iterator variable names. Probably I have optimized the code and lost a, b and c.

Regarding long variable names: Having a variable name that is describes what it is being used for is good. I tend to prefer to have a comment describing what the variable does along with a shorter name, unless it is important to keep track of the meaning of the variable.

I personally find it more readable when long variable names are declared. But I can follow your suggestions for future PRs in humlib.