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

Non applicable #14 #30

Merged
merged 9 commits into from
Jul 6, 2015
Merged

Non applicable #14 #30

merged 9 commits into from
Jul 6, 2015

Conversation

jackfirth
Copy link
Owner

No description provided.

@AlexKnauth
Copy link
Collaborator

Even if lenses aren't two-value functions any more, it still makes sense to keep the lens-struct representation the same, because that way the function can share computations between the getter and the setter. I'll make another simpler pull request to do that, based on this one.

@jackfirth
Copy link
Owner Author

Computation-sharing can be supported in a separate issue (I'll make one to track it). It's not always better to share computation - all cases where only getting or setting is needed take on the penalty of computing what's needed for the other half. However, by not exposing the struct at all and only exposing make-lens, apply-lens, and let-lens, at a later date an alternate representation that stores a function that computes both at once can be used, and a version of make-lens that takes such a sharing function could be added. I'd also really like some data on performance-intensive uses of lenses and where the bottlenecks arise.

@AlexKnauth
Copy link
Collaborator

Well, isn't it simpler to keep the representation the way it is now, and possibly add your new one later?

Plus, the getter will not have to compute any setter-specific information unless you define it that way, but the setter can take advantage of computations already done by the getter.

@jackfirth
Copy link
Owner Author

The representation isn't really the problem, the problem is that all other lens-defining modules are explicitly depending on that exact representation. That locks me out of ever being able to change that representation. I imagine for some lenses the separate getter and setters would be more performant than sharing. For now I want all lenses to only be defined in terms of make-lens. I've also found it makes the implementation of most lenses clearer and is a nicer API to use.

Bottom line, I want to get rid of implementation-dependent constructions of lenses until I figure out how to properly support computation-sharing lenses.

jackfirth added a commit that referenced this pull request Jul 6, 2015
@jackfirth jackfirth merged commit 01864e7 into master Jul 6, 2015
@AlexKnauth
Copy link
Collaborator

If they use make-lens, which is already define in terms of the lens-struct and the 2-value function, then they aren't depending on that representation, and it shouldn't slow down the getter very much.

However, if you want to make the setter more efficient by re-using computations already done by the getter, then the previous representation is the best one.

@jackfirth
Copy link
Owner Author

Agreed, and I plan to support that, but in a way better than the previous representation. There's much to do in the realm of optimization, and I'm hesitant to devote much time or complexity to it before committing to a 1.0 API

@AlexKnauth
Copy link
Collaborator

Well, changing it back isn't adding any complexity, really: #33
In fact it takes one less line of code, for a more flexible and efficient representation.

@jackfirth
Copy link
Owner Author

As I said, I don't want to do it yet until I have more information about the relative costs of computation sharing and what other optimizations are possible. For instance, with separated lenses lens-compose can be optimized to compose the underlying getters and setters independently. The performance implications and possible optimizations are not straightforward, neither is directly more flexible and efficient than the other in all circumstances. For that reason, I'm sticking with separated lenses to keep the implementation simple for 1.0, then looking into optimization later.

@AlexKnauth
Copy link
Collaborator

A possible solution:

Would it be a good idea to make this into a gen:lens generic interface with the methods lens-view, lens-set, and apply-lens?

That way the interface stays the same, but different implementations can take different trade-offs, and functions like lens-compose could, in the future, recognize specific structs as special cases to optimize for?

@jackfirth
Copy link
Owner Author

Yes, that's a good idea, and wouldn't break backwards compatibility. I'm primarily concerned with getting the 1.0 api documented and solidified first however, so that will have to be a post-1.0 addition.

@AlexKnauth
Copy link
Collaborator

Ok, but would lens-view/context be a better name than apply-lens, now that lenses have been disconnected from 2-value functions?
(By the way that's that I do in #37)

@jackfirth
Copy link
Owner Author

Yes, apply-lens is a bad name now. Will make an issue tracking that and schedule a fix.

@AlexKnauth
Copy link
Collaborator

The representation isn't really the problem, the problem is that all other lens-defining modules are explicitly depending on that exact representation. That locks me out of ever being able to change that representation.

If you don't want lens modules to depend on the representation, and especially if you want to avoid being locked out of being able to change it, then it seems like a generic interface is exactly what you want. That's what gave me the idea.

@jackfirth
Copy link
Owner Author

It is, with the exception that if I actually expose and document the generic interface I'm locked into the fact that it must be generic as well, as clients of the package could make arbitrary representations of lenses. I am yet not sure if that extensibility is a good idea. All I know 100% for certain at this point is that I want to be able to pass a getter and setter to make-lens and get back a lens, so that's all I'm committing to right now.

@AlexKnauth
Copy link
Collaborator

Would it work if we only exposed it in the unstable collection?

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.

None yet

2 participants