Skip to content

Conversation

@dleve123
Copy link

@dleve123 dleve123 commented Nov 1, 2021

Problem:

The docs as currently written are a bit misleading as they suggest that
these higher-order functions returns Tensors directly, when in fact
they return functions that return Tensors.

Solution:

  • Update function docs to reflect that these functions return functions
  • Add type annotations for higher-order functions to reinforce the above (happy to remove this we don't want to piece-meal introduce types to minitorch)

The docs as currently written are a bit misleading as they suggest that
these higher-order functions returns `Tensor`s directly, when in fact
they return functions that return tensors.
Returns:
:class:`TensorData` : new tensor data
function: A function that takes a tensor, applies `fn` to each cell,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon digging deeper here, it seems like the Args docs mix the arguments for the higher-order function and the function it returns...would it make sense to move the docs for a and out to be sub-points of this doc line (and so-forth for the other higher order functions).

@dleve123
Copy link
Author

dleve123 commented Nov 2, 2021

@srush ping on this whenever you have a sec! Happy to update the docs for fast_ops accordingly if we are aligned here (in this PR or a follow up one).

@srush
Copy link
Contributor

srush commented Nov 2, 2021

I think it is clear as written. Would rather at types everywhere or not at all.

@dleve123
Copy link
Author

dleve123 commented Nov 2, 2021

Would rather at types everywhere or not at all.

For sure – I found it personally helpful to "type out" the more conceptually complicated functions like these higher-order ones, but agree it might not be valuable to introduce types bit by bit.

I think it is clear as written.

To respectfully push back: As a learner, I think it's a bit confusing that the docs for the higher-order functions in operators.py document that these functions return other functions, but not here. Maybe I'm missing something, but they should be conceptually the same (except one operates on lists, the other tensors). Example in operators.py:

"""
Higher-order map.
.. image:: figs/Ops/maplist.png
See `<https://en.wikipedia.org/wiki/Map_(higher-order_function)>`_
Args:
fn (one-arg function): Function from one value to one value.
Returns:
function : A function that takes a list, applies `fn` to each element, and returns a
new list
"""

Happy to close the PR, just wanted to clarify my reasoning here!

@srush
Copy link
Contributor

srush commented Nov 4, 2021

Aren't these docs the same though as Module-2? Is it specific to fast_ops?

@dleve123
Copy link
Author

Sorry for the delay – was pretty heads down on Module-3 :)

Aren't these docs the same though as Module-2?

Yes, they are the same as Module-2 – I just noticed when working on Module-3, but the same comment/suggestion applies for Module-2.

Is it specific to fast_ops?

Nope, similar comments apply for cuda_ops.


Perhaps the goal here is to document the pipeline of calling the higher-order function and then calling the returned function all in one go, like you do here?

fn_reduce = reduce(fn)
out = fn_reduce(a, dim)

If so, happy to leave as well – just tripped me up briefly when reading the docstrings!

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.

2 participants