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

Deprecate fmap(walk, f, x, ys...) for execute(walk, x, ys...) #65

Merged
merged 3 commits into from
Jun 24, 2023

Conversation

gaurav-arya
Copy link
Contributor

@gaurav-arya gaurav-arya commented Jan 19, 2023

Deprecates fmap(walk, f, x, ys...) for runwalk(walk, x, ys...), with no higher ambitions. Resolves #62

Edit: runwalk now renamed to execute

@gaurav-arya
Copy link
Contributor Author

Closing per discussion in #62

@gaurav-arya
Copy link
Contributor Author

Well, reopening in case the little bit of work I did with the deprecation becomes something we want to use in the future, but this PR isn't ready for merge.

@gaurav-arya gaurav-arya reopened this Jan 21, 2023
@darsnack
Copy link
Member

darsnack commented Apr 12, 2023

I think we should go ahead and do this, but use traverse and note in the docstring that this is post-order traversal.

Just getting rid of the confusing fmap(walk, f, ...) method will be an improvement even if the naming isn't perfect. I imagine a proper renaming that mirrors prior literature on "reduce," "traverse," "fold," "map," etc. will be bigger, breaking overhaul anyways. And so the name we choose here won't make it significantly more painful that it is going to be anyways.

@gaurav-arya
Copy link
Contributor Author

gaurav-arya commented Apr 12, 2023

Sounds good! Happy to do a renaming. I'm musing about whether traverse is the appropriate name, though. I don't speak Haskell, but from https://stackoverflow.com/questions/7460809/can-someone-explain-the-traverse-function-in-haskell it looks like it's an fmap that guarantees a particular ordering and allows effectful functions. In particular, it seems like it should definitely have the same signature as fmap (and the stack overflow answer notes that the distinction between fmap and traverse is blurred in an impure language.)

So maybe traverse isn't the best name for this function? To me, it feels like this a lower-level primitive which is very much to do with the specific concept of a walk in this library, So maybe something like dowalk to avoid the oxymoron? Very happy to be contradicted and for other suggestions in the name.

Edit: I guess your point probably is that we should just use traverse because it's a fitting name, and ignore any FP comparisons. I mostly agree, but I wonder if it's still better to avoid using that specific name which already has other implications, to avoid any confusion

@darsnack
Copy link
Member

darsnack commented Apr 17, 2023

How about descend(walk, x, ys...) (or execute or enter)?

But yes, my point was that the closest FP concept to what's going on is a post-order traversal, so we should just use that.

@CarloLucibello
Copy link
Member

@gaurav-arya let's decide a name and merge this, if you have some time

Copy link
Contributor Author

@gaurav-arya gaurav-arya left a comment

Choose a reason for hiding this comment

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

Sure -- have gone with execute

docs/src/api.md Outdated Show resolved Hide resolved
src/Functors.jl Outdated Show resolved Hide resolved
src/Functors.jl Outdated Show resolved Hide resolved
src/maps.jl Outdated Show resolved Hide resolved
src/maps.jl Outdated Show resolved Hide resolved
src/maps.jl Outdated Show resolved Hide resolved
src/walks.jl Outdated Show resolved Hide resolved
src/walks.jl Outdated Show resolved Hide resolved
src/walks.jl Outdated Show resolved Hide resolved
@gaurav-arya gaurav-arya changed the title Deprecate fmap(walk, f, x, ys...) for runwalk(walk, x, ys...) Deprecate fmap(walk, f, x, ys...) for execute(walk, x, ys...) Jun 24, 2023
src/walks.jl Show resolved Hide resolved
@darsnack darsnack merged commit 9ea11f1 into FluxML:master Jun 24, 2023
6 checks passed
@darsnack
Copy link
Member

Thank you @gaurav-arya for all the hard work and thoughts here!

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.

fmap API that accepts an arbitrary walk does not use function f
3 participants