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

Handle NumPy types #48

Closed
cifkao opened this issue Jan 8, 2021 · 9 comments
Closed

Handle NumPy types #48

cifkao opened this issue Jan 8, 2021 · 9 comments

Comments

@cifkao
Copy link
Contributor

cifkao commented Jan 8, 2021

MusPy currently doesn't consider NumPy types to be valid:

music.adjust_time(lambda t: t - np.random.choice(6))
music.validate()  # Oops, np.int64 is not int
@cifkao
Copy link
Contributor Author

cifkao commented Jan 8, 2021

Since version 1.9.0, NumPy integers are subclasses of numbers.Integral. So we can just replace int with that basically everywhere.

@salu133445
Copy link
Owner

I think numpy>1.9 is already enforced in setup.py by Pypianoroll, which requires numpy>1.12. Do we need any other changes in addition?

@cifkao
Copy link
Contributor Author

cifkao commented Jan 11, 2021

We would need to replace int with numbers.Integral in all attributes.

@salu133445
Copy link
Owner

That doesn't look safe to me. We will need to add int() in many places (e.g., MIDI output using mido) if so. Instead, I would suggest to provide some way to convert NumPy numbers to Python numbers.

Seems relevant - python/typing#272.

@salu133445
Copy link
Owner

But maybe we should support NumPy numbers as they are quite common. We could keep a stricter type on the attributes but add int() to places that would require a Python int to work properly (e.g., MIDI output). We might need some test cases that use NumPy numbers to see where we need an int().

@cifkao
Copy link
Contributor Author

cifkao commented Jan 12, 2021

One can very easily introduce NumPy numbers without realizing it. The way I found out about this was when I called adjust_time with a function that used something from NumPy, and then remove_invalid basically removed everything. It took me some time to figure it out. So I think the code should try to work correctly with NumPy numbers regardless of type annotations.

One way would be to use Integral in _attributes, but keep int in argument/return type hints. This way the code will work correctly for people who don't use static type checking. And static type checking should work the same way as it does now (i.e. you might get an error if you try to assign a NumPy int – though I'm not sure what is the status of type annotations in NumPy).

Why exactly does MIDI output require Python ints? Does mido do some type checks?

@salu133445
Copy link
Owner

Actually, duck typing is the principle and type checking is optional. And, for this reason, I think we should keep type annotations as strict as it can be. Some possible solutions:

  • Try to fix such 'fixable' errors in remove_invalid.
  • Add Music.fix() that automatically fixes these 'fixable' errors.
  • Add int() in places that would otherwise raise errors.

I guess so. I remembered either mido or pretty_midi throws an error when I passed a NumPy scalar. Another example is that NumPy integers are not JSON serializable, so dumping them would raise an error.

@cifkao
Copy link
Contributor Author

cifkao commented Jan 13, 2021

Good point about the serialization...

Then I guess the safest thing to do would be to make sure that remove_invalid (and other functions where types matter) raises an exception with NumPy scalars (and probably any other invalid types) instead of silently removing them. At least by default. And all input functions should make sure they return objects with correct types.

For example, looking at from_note_representation, I think it will actually introduce NumPy scalars. And the same for the current implementation of from_event_representation. Also, from_dict should either check or coerce the type here.

salu133445 added a commit that referenced this issue Aug 17, 2021
salu133445 added a commit that referenced this issue Aug 17, 2021
@salu133445
Copy link
Owner

The Base.from_dict method now have the strict and cast options.

  • cast: cast any bad input type to the correct type
  • strict: raise a TypeError for any bad input type

Also, the Base.fix_type will fix the types.

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

No branches or pull requests

2 participants