MarkGotham / When-in-Rome

meta-corpus of and code library for the functional harmonic analysis of music
58 stars 12 forks source link

ABC v2, misc other #42

Closed malcolmsailor closed 2 years ago

malcolmsailor commented 2 years ago

This pull request has three parts:

  1. implementation of convert_DCML_tsv_analyses
  2. newly converted ABC analyses
  3. a few other corrections I had made to some of the other analyses (e.g., "VIII" -> "VII", "IV/64" -> "IV64").

I hope the inclusion of the last item doesn't make the pull request too confusing. I didn't realize those other changes were there until after committing items 1 and 2.

By the way, @MarkGotham , did you use the v2_preview for the DCML files? I believe that is the latest version.

MarkGotham commented 2 years ago

Thanks for this @malcolmsailor !

  1. Great. You ask about checks on corpus validity. The updates_and_checks.py file does this with an explicit list at the top and then the check itself is in line 73 (of the pre.PR version).
  2. Great, though it looks like some issues remain with the converter which we can discuss over on that issue/discussion in m21.
  3. Thanks, there are several fixes of obvious syntax errors in there so that's great and always welcome. That said, a lot of 'changes' are coming up without any apparent change and there are some that are Updated upstream (since Dmitri sent in a corpus update). E.g. Early_Choral/Monteverdi,_Claudio/Madrigals_Book_3/18/analysis.txt please can you investigate those cases and remove as appropriate.

Finally

malcolmsailor commented 2 years ago
  1. Great, though it looks like some issues remain with the converter which we can discuss over on that issue/discussion in m21.

Yes, we can rerun the code and re-commit after the converter is finalized. Actually I think the code to do so should be included in the repository so the user can just update it themselves. As far as I can tell, you are calling updates_and_checks.py from other scripts or interactive sessions that are not actually included in the repository itself? That's what I did, since that appeared to be what you were doing.

  1. Thanks, there are several fixes of obvious syntax errors in there so that's great and always welcome. That said, a lot of 'changes' are coming up without any apparent change and there are some that are Updated upstream (since Dmitri sent in a corpus update). E.g. Early_Choral/Monteverdi,_Claudio/Madrigals_Book_3/18/analysis.txt please can you investigate those cases and remove as appropriate.

I merged the upstream in 20 hours ago. Has Dmitri sent changes since then? I accepted all the upstream changes, but for some reason that I don't understand that one slipped through. (It seems to me that git shouldn't have allowed me to commit without correcting the conflict.) I just fixed it with a new commit.

The 'changes' without apparent change appear to be due to my text editor adding a line-break to the end of the file. Sorry about that, although I believe it is probably a good thing to have every file end with a line-break.

MarkGotham commented 2 years ago

Hi @malcolmsailor, thanks!

Yes indeed, the idea of updates_and_checks.py is to make the system completely transparent and reproducible. It's a work in progress in which I aim to:

If you see anything missing then let me know.

I'm content to merge work in progress but the beat errors are a bit too much of an obviously backward step to countenance ;) The issue is fixed now right? So please can you include that here? Then I'll merge even if/though there's further work to be done.

Cheers!

malcolmsailor commented 2 years ago

Yes indeed, the idea of updates_and_checks.py is to make the system completely transparent and reproducible. It's a work in progress in which I aim to:

OK, but updates_and_checks.py is not executable, containing only function definitions, right? When I processed the ABC corpus I actually added something like if __name__ == "__main__": convert_DCML_tsv_analyses(). Why not make it executable and use argparse or similar to allow the user to run the various functions?

  • exclude anything specific to my (or any individual's) system.

What sort of thing would that be? Like the paths to the other corpora for processing? I think the appropriate way of doing that would be to use environment variables.

I'm content to merge work in progress but the beat errors are a bit too much of an obviously backward step to countenance ;) The issue is fixed now right? So please can you include that here? Then I'll merge even if/though there's further work to be done.

Yes agreed. But maybe it would be a good idea to have a development branch for works-in-progress like this, so we can merge in contributions that still need work?

Cheers!

MarkGotham commented 2 years ago

Sounds good!

malcolmsailor commented 2 years ago

Just pushed a new commit that rebuilds the ABC corpus with the current version of the music21 tsv converter.

I also added some lines to the bottom of updates_and_checks.py. My suggestion would be that we add arguments to the argparser to invoke the various other functions to build/update the corpus. We might consider moving this code into a separate file called something like "build_corpus.py" or even "when_in_rome.py" in the base directory of the repository, that then imports functions from updates_and_checks.py and anything else it needs.

MarkGotham commented 2 years ago

Great, thanks!