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 simplified syntax: arr: NDArray["2, 2", int]? #92

Closed
petered opened this issue Sep 6, 2022 · 4 comments
Closed

Allow simplified syntax: arr: NDArray["2, 2", int]? #92

petered opened this issue Sep 6, 2022 · 4 comments

Comments

@petered
Copy link

petered commented Sep 6, 2022

NPTyping is great, but could be more concise.

The main value of NDArray to me is to have a very concise way of documenting shapes.

I would propose

  • Just allowing us to enter a string or tuple for the shape, e.g. NDArray["2,2", Int] or NDArray[(2, 2), Int], as opposed to the more verbose NDArray[Shape["2,2"], Int]
  • Allowing us to just use built-in numeric types instead of importing them from nptyping. E.g. int vs nptyping.Int, etc.

I'd be happy to make a PR to this effect if it is approved in spirit.

@ramonhagenaars
Copy link
Owner

Hi @petered ,

Concerning your two proposals:

  • The syntax NDArray[<shape-expression>, <dtype>] (not wrapping the former in a Shape) will not work well with MyPy. The current 2.+ syntax has been carefully put together to make it acceptable by MyPy while also keeping auto-completion on numpy arrays. This may seem easier than it is, it's been a pain... if you somehow can think of a trick to make that syntax work while keeping auto-completion and MyPy, I am actually quite interested.
  • This could work, although it may break MyPy if you use it, for int not being a dtype. I'm not so sure if the benefit of not having to import Int outweighs the breaking of MyPy for the user and the extra code to maintain for this library. There have been no other feature requests for this matter.

@petered
Copy link
Author

petered commented Sep 9, 2022

Hi Ramon, thanks for your response.

I don't understand MyPy enough to know the difficulty involved, and honestly probably won't have the time to invest to see if it's doable.

For me - the primary value of nptyping is just to have consistent documentation - I don't even run mypy.

Perhaps, assuming other people are using NDArray like I am - just as a form of documentation, we could add an experimental type that could later be merged into NDArray if mypy allows. Something like:

import numpy as np
from nptyping.experimental import Array

def adjust_brightness(rgb_image: Array["H,W,3", np.uint8], brightness_map: Array["H,W", float]
    ) -> Array["H,W,3", np.uint8]:
    ...

What would you think of this idea?

@ramonhagenaars
Copy link
Owner

Hi there Peter,

If documentation is the only thing you're after, then maybe aliasing literals may be sufficient for you? It would save you a dependency and you would be free to use Python primitives as well. You could still choose to use the nptyping shape expression syntax:

from typing import Literal as NDArray


arr: NDArray["H, W, 3", int]

What this lacks is that there is no checking of that syntax of course, but you could do that "offline" in a repl.

You could also alias nptyping.Shape to make it a bit less prominent:

from nptyping import NDArray, Int, Shape as _


arr: NDArray[_["H, W, 3", Int]]

I actually plan to promote this last trick for the upcoming nptyping.DataFrame that would look a bit convoluted otherwise.

I don't understand MyPy enough to know the difficulty involved

The difficulty is that MyPy simply regards a piece of string inside any type that is not a typing.Literal invalid. The same goes for IDEs such as Pycharm. If there is a string in a hint, it must be in typing.Literal. Otherwise, type checkers try to interpret the text as a type - e.g. list["int"] works perfectly fine. For this reason, I don't think this is going to change any time soon and therefore I'm a bit hestitant to add the experimental feature that you describe.

@ramonhagenaars
Copy link
Owner

No more recent activity: closing.

@ramonhagenaars ramonhagenaars closed this as not planned Won't fix, can't repro, duplicate, stale Feb 21, 2023
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