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

Allow iter_all to iterate over multiple classes #428

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

leleogere
Copy link
Collaborator

Fixes #426.

When iterating over a Part's elements, the user can only provide a single class to filter the output. If they want to get multiple classes, they can either iterate over all classes and filter a posteriori, or iterate multiple times over a single class. Those two workarounds are quite slow, so the possibility to iterate over multiple classes in a single pass would be useful.

This PR simply allows the user to pass an iterable to the cls parameter of iter_all. For example, the following would allow to easily retrieve a list of all key/time signature/clef change over the score (in that order when they are simultaneous), in a single iteration over all time points.

part.iter_all(cls=[TimeSignature, KeySignature, Clef])

There are still a couple of things to discuss before merging this, briefly evoked in #426:

  • It makes sense that the parameters start, end and mode apply to all classes. However, should the parameter include_subclasses apply to all of them? Maybe I could add the possibility to pass either a single boolean (apply to all classes) or an iterable of booleans (apply to indivisual classes, same size as the cls iterable).
  • What if a user provides cls=[GenericNote, Note] with include_subclasses=True? (Or even simpler if they provide cls=[Note, Note]) Should the iterator return duplicate objects? Or deduplicated objects only? Or raise an error?

I have not added any tests yet, but I can add some, although I'm not sure in which file they would belong. I haven't found any existing test for the function iter_all.

@manoskary
Copy link
Member

Hello @leleogere that is a very nice and cool addition to the package. May I ask you to add a test for that, it does not have to be much, you can use partitura.EXAMPLE_MUSICXML and iter through that, no need to create a new test file you could add it to an existing one. Thanks!!!

@leleogere
Copy link
Collaborator Author

leleogere commented Mar 10, 2025

Thank you @manoskary! Yes I'll add some tests!

Before that, do you have some insights about the two points I raised?

There are still a couple of things to discuss before merging this, briefly evoked in #426:

  • It makes sense that the parameters start, end and mode apply to all classes. However, should the parameter include_subclasses apply to all of them? Maybe I could add the possibility to pass either a single boolean (apply to all classes) or an iterable of booleans (apply to indivisual classes, same size as the cls iterable).
  • What if a user provides cls=[GenericNote, Note] with include_subclasses=True? (Or even simpler if they provide cls=[Note, Note]) Should the iterator return duplicate objects? Or deduplicated objects only? Or raise an error?

@manoskary
Copy link
Member

Hello @leleogere thanks for your message. Let me reply here.
If feels like all proposed solutions are valid but I think in this case, choosing the most straightforward solution for the user would be best in my opinion. I would leave the include_subclasses as a bool instead of a list, to facilitate user based implementation. If you require such an action, where you would like to specify for each class individually options for it, then probably individual iters would be the solution.

For the case where cls=[GenericNote, Note] with include_subclasses=True or cls=[Note, Note] we should not return duplicates. Then the decision is in your hands, you could either raise an error or curate the user's input and raise a warning.

But that is just my opinion on this topic.

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.

Allow cls parameter of iter_all to take an iterable of classes rather than a single class
2 participants