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

Made last set of positional arguments available in DynamicMap #719

Closed
wants to merge 1 commit into from

Conversation

jlstevens
Copy link
Contributor

This PR is intended to address issue #705

@philippjfr It is a simple change but it would be good if you could review it anyways.

@philippjfr
Copy link
Member

Not sure I'm entirely happy with the name args, it really doesn't tell me anything, last_args might be okay but really it's the last generated key, calling it args seems confusing. Maybe it would make sense to change the semantics of last and last_key on DynamicMaps to point to the most recently generated objects rather than the last one in the sorting order?

@jlstevens
Copy link
Contributor Author

Maybe it would make sense to change the semantics of last and last_key on DynamicMaps to point to the most recently generated objects rather than the last one in the sorting order?

I would prefer to keep DynamicMap as similar to HoloMap as possible. I'm happy to call it last_args if you prefer.

The reason I think args is the correct name is that args and kwargs are the standard names when considering callables. Talking about 'keys' in this case seems like it would be confusing to me...

@philippjfr
Copy link
Member

The reason I think args is the correct name is that args and kwargs are the standard names when considering callables. Talking about 'keys' in this case seems like it would be confusing to me...

Don't really agree, our NdMapping based datastructures are about keys and adding another name for what is really just the last generated tuple key makes it seem like a separate concept when it really isn't. The DynamicMap is asked for a key and that key is passed as the args to the callback, so the args is a concept the callback worries about but at the DynamicMap level we should be talking about keys.

@jlstevens
Copy link
Contributor Author

jlstevens commented Jun 17, 2016

... at the DynamicMap level we should be talking about keys.

If you like, but I don't think we should change the semantics of last or last_key. Do you have another suggestion for a name?

@philippjfr
Copy link
Member

If you like, but I don't think we should change the semantics of last or last_key.

Yea, you're right, probably not a good idea. I'll think about it.

@jbednar
Copy link
Member

jbednar commented Jun 17, 2016

most_recent_key? current_key?

@jlstevens
Copy link
Contributor Author

last_executed_key?

@jlstevens
Copy link
Contributor Author

I think that streams make this a bit more complicated as there are now *args (anything in kdims, whether streams or not) or **kwargs which are streams not in the kdims. Only the *args part could be considered a key...

@philippjfr
Copy link
Member

Should we close this? With addition of Callables this is probably quite out of date. I wouldn't mind if we made the last key and stream values available on the Callable though.

@jlstevens
Copy link
Contributor Author

I wouldn't mind if we made the last key and stream values available on the Callable though.

That would be reasonable and yes, I agree this original approach is now out-of-date. I don't think the Callable should know about keys versus streams but the arguments for the last call should be made available somehow.

@philippjfr
Copy link
Member

Opened an issue for the suggestion and closing this PR as outdated.

@philippjfr philippjfr closed this Apr 18, 2017
@philippjfr philippjfr deleted the callback_args branch August 21, 2017 20:59
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants