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
Generic einops function which infers which operation to call #84
Comments
Yes, this is possible -- but should it be done? What are the actual use cases for this "all in one" function? I really like descriptive function names like |
@MilesCranmer interesting suggestion, but I think in the balance of verbosity and shortness of code einops should be on verbosity side. Thinking of user base, right now most people when they see einops code, they see it for the first time. Let's try to make that experience smooth. Explicit wordy clues what operation actually does are very helpful (e.g. repeat tells what's happening, so if I had no idea about einops - that's the minimal thing I will certainly understand in code). Until einops is wide-spread enough, I'd hesitate to introduce shortcuts. Open to hear other opinions (PR is well-written BTW, didn't mean to discourage you from contributing) |
Thanks for the comments! @shoyer I first note that this function division isn't as clear as you might think. For example, I can do However, the above argument is more subjective, and more importantly I note that this added function doesn't replace anything - all those functions are still there and their use can be encouraged for first-time users. The generic operation is just there for power users who would rather read the pattern than the function name, such as myself. And if one does have the opinion that three separate functions are more readable to external readers, then they are free to continue using those functions in open-sourced code. Third, I note that Interested to hear thoughts! |
Also, one tiny additional comment: my brain (and vim's syntax highlighter) confuse However, seeing But in general I am fairly new to API crafting, and I will trust your final opinions on this! *The einops pattern could be considered as the function in this case. |
I think having a single operation would be a nice addition actually because (as Miles said):
|
^Completely agree! Seeing Re: functional division, here's some examples of where the function choice is not completely clear, but the pattern is very obvious:
I also think that to a beginning user, "reduce" is a bit of an ambiguous term: some might say that the operation |
My comments:
... which does not help much with its adoption. It's around since (sic!) 2011, but still not as widely used as it should be. It is wrong to think about 'API for new users' and 'API for professionals'.
Name conflicts:
Some other considerations:
|
Sorry for the late reply. No problem at all @arogozhnikov, those are good arguments and I agree there are pros and cons to introducing this feature. Happy to submit an updated PR in the future should this come up again, as I'm still very interested in such a function. Feel free to close until then. Cheers, |
My 2 cents: In my initial experience with einops the existence of multiple functions was actually confusing me instead of being useful. Like in einsum, I expected the einops "language" to just let me express how the data was now and what I wanted it look after and it should just do it, I believe the language is fairly intuitive. As @MilesCranmer pointed out, the current division can sometimes be confusing, for example say you started with code like this: x = repeat(x, "h w c -> batch c h w", batch=32) Here you are tiling in the batch dimension but also transposing the channel dimension. Now imagine that for some reason you don't want to do the tiling in the x = repeat(x, "h w c -> c h w") This look good to the eye but its wrong because x = rearrange(x, "h w c -> c h w") |
Hi @arogozhnikov et al, Just wondering if any of you have any updated thoughts on this? I'm still eager to have this functionality available. I've introduced 10ish people to einops over the past year and every time I do, it feels like the missing piece for having einops be a 100% intuitive package is a single function like einsum. Each time I demo the einops API, I have to explain that all functions pretty much work the same, the different function names are effectively just a way of documenting the code. Being explicit is great, but when so much functionality overlaps between the functions, to me it sticks out like a code smell to have different function names altogether. But of course, this is all subjective. Anyways, in recent days I've been reading through the jax source for einsum and then remembered this issue and PR; it seems the method used for inferring whether to do reductions is inferred in a similar way by simply checking uniqueness of indices; e.g., https://github.com/google/jax/blob/62230f65256728f580c5ecfa8867cac69a681cb1/jax/_src/lax/parallel.py#L408. Cheers, |
I am still very interested in this! Comes to mind everytime I use the library. |
At the risk of looking stubborn: My previous points are still there (verbosity), and the main target auditory still consists of people with numpy/matlab experience and style of thinking. I see tremendous progress in this direction over the past year, but we're not there to say 'einops is default knowledge'. When thinking in previous paradigm, you mentally first decide operation, not result. Unwrapping my previous argument about mixing reduce/repeat: einop('i j toekn -> j token', 'min') Fragile and hard to parse. But if you forbid this - consistency is lost and there are 'right' and 'wrong' patterns with no trivial rule to tell one from the other. Complaining meaningfully about patterns also becomes hard. At the same time Other related thoughts:
👍 thank you @MilesCranmer ! |
Okay, no problem, just wanted to ping the thread! I should mention I'd even be happy with a
I think these sorts of requirements should be performed by a linter, rather than at the API level. If I want, I can define all my variables with single character names - python will still run (as I think it should!) - but my linter would complain. I think the same goes for good patterns and good function names - personal choices shouldn't be enforced by the API, but rather by a python linter, or by someone reviewing a pull request. I do like the sound of using capital names for new axes. I think I'll do that from now on actually! (But I agree, it shouldn't be enforced by any API.) |
Pinging this thread again @arogozhnikov to see if your views have changed with increasing use of einops. It sounds like a lot of people would be interested in this. Also - resolved merge conflicts in the PR. Cheers, |
Nobody mentioned it, but just in case einop is available in separate repo: |
It’s definitely nice to have @cgarciae’s package but fragmentation of packages into smaller libraries is perhaps not great, esp. for downstream maintainability. A single |
To both points, separate package is the right solution. |
Does this issue also include adding this functionality into |
I noticed that
rearrange
,repeat
, andreduce
can all be inferred based on the pattern alone. Therefore a single generic function which makes calls to each is possible. I think this would be very nice for power users! For example,Here is how each operation can be inferred:
i j k -> k j i
, ortime (i j) -> i j time
are both recognized as patterns for rearrange.i j -> i
, so obviously a reduction. Similarly,(h1 h2) (i j) -> h1 i
.i -> i j
, so obviously a repeat. Ortime i k -> time i h k
An error is raised for any of the following situations:
axes_length
These would prevent unintended behavior. Errors could also be raised for when a user gives a value for
axes_lengths
, but does not have that index inside the equation.I would be happy to implement this (and also #83). Let me know if interested.
Also would be nice for if #73 is included as well. Then this operation could be
einop(x, [y, ], pattern, ...)
, with the einsum function inferred by the presence of two input tensors.The text was updated successfully, but these errors were encountered: