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

Cleanup of the plotables module #870

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Conversation

pfebrer
Copy link
Contributor

@pfebrer pfebrer commented Nov 13, 2024

This fixes a bug: one could not pass **kwargs to the data generators when plotting through plot handlers.

Now:

# Passing kwargs to data from the plot handler
object.plot(data_kwargs={"arg2": 2})

# Is equivalent to
data = DataClass.new(obj, arg2=2)
plot = PlotClass(data)

in other words, data_kwargs is correctly expanded to pass the **kwargs for the creation of the data.

Most importantly, this PR rewrites the _plotables.py with plenty of documentation so that it is easier to modify in the future.

func1_var_kwarg = None

# Loop through the parameters of the first function
for param in list(signature1.parameters.values())[func1_slice]:

Check failure

Code scanning / CodeQL

Unhashable object hashed Error

This
instance
of
slice
is unhashable.
This
instance
of
slice
is unhashable.
This
instance
of
slice
is unhashable.
@zerothi
Copy link
Owner

zerothi commented Nov 13, 2024

Thanks @pfebrer, perhaps we should have some tests that reflects what is possible, and how they should work. I'll merge this, and work on my other tbtrans branch. Thanks!

@zerothi zerothi merged commit 04c70d1 into zerothi:main Nov 13, 2024
14 of 15 checks passed
@pfebrer
Copy link
Contributor Author

pfebrer commented Nov 13, 2024

Yes, definitely, and we should probably also discuss the behavior of **kwargs.

Right now, nodify workflows don't support **kwargs (nodes do support them). Therefore plots are not allowed to have **kwargs for now. So perhaps the best idea is to just pass the kwargs to the data function if they are declared. 🤷

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