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

Plot attributes #848

Merged
merged 42 commits into from
Sep 1, 2023
Merged

Conversation

FranzForstmayr
Copy link
Collaborator

@FranzForstmayr FranzForstmayr commented Feb 13, 2023

This PR tries to get rid of the code in

# TODO: remove this as it takes up ~70% cpu time of this init
which generates the plotting methods for Networks and adds them to the network object.

Due to less code parsing the initialization time should be reduced, as the method itself is now parsed only once (plot_attrribute), for the individual plotting methods like plot_s_re,plot_s_db and so on, the method is called with different parameters.

For all the autogenerated methods a new docstring is created. There's no way to check the correctness of the plot, however we can see if a exception gets thrown.

@github-actions github-actions bot added the Media label Mar 9, 2023
@FranzForstmayr
Copy link
Collaborator Author

NetworkSet still has some strange dynamic plotting methods, but plotting.py is much cleaner now.

@FranzForstmayr
Copy link
Collaborator Author

We should really ask our self, if we need that much methods on a class like NetworkSet.
Before the autogeneration of attributes in NetworkSet's constructor the __dir__() object has 81 entries. This is not less, but ok.
After the generation we have 952!! entries, most of them autogenerated plot methods, in __dir__().

This PR should not make any functional difference to any user, but we really need to tighten the API in the future.

@jhillairet
Copy link
Member

NetworkSet still has some strange dynamic plotting methods, but plotting.py is much cleaner now.

There are the same changes in the PR #852, could say essentially what/where are the difference? I'm a bit lost...

We should really ask our self, if we need that much methods on a class like NetworkSet.

This is probably too much you're right, but it's hard to say what users are really using or not....

@FranzForstmayr FranzForstmayr marked this pull request as ready for review June 17, 2023 12:05
@jhillairet
Copy link
Member

what about this one for the next release ?

@FranzForstmayr
Copy link
Collaborator Author

There are some docstring which need some more work. I think we should wait for the next one.
Furthermore I wanted to create unittest for the plot functions, they should not raise something unexpected

@FranzForstmayr FranzForstmayr marked this pull request as draft July 12, 2023 06:36
@FranzForstmayr FranzForstmayr marked this pull request as ready for review July 17, 2023 23:05
@jhillairet
Copy link
Member

@FranzForstmayr could you look to the conflicts and make a brief summary of the purpose of the the changes ?

@FranzForstmayr
Copy link
Collaborator Author

I'm ready to merge if ok

@jhillairet jhillairet merged commit e1724ad into scikit-rf:master Sep 1, 2023
11 checks passed
@jhillairet jhillairet mentioned this pull request Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants