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

Improve performance network.copy #875

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

eendebakpt
Copy link
Contributor

The network.copy makes a deep copy of the frequency object, and this copy happens again in the Network constructor.
This PR removes the explicit copy of the frequency object and adds a test.

Note: are the copies at https://github.com/scikit-rf/scikit-rf/blob/master/skrf/calibration/calibration.py#L245 required? If not, we could remove them as well

@jhillairet
Copy link
Member

I guess that, originally, the copy() was made to avoid modifying the frequency objects of the copies if the original one was modified... Can you check it is not the case after the change?

@eendebakpt
Copy link
Contributor Author

I guess that, originally, the copy() was made to avoid modifying the frequency objects of the copies if the original one was modified... Can you check it is not the case after the change?

I added test cased to cover this

@FranzForstmayr
Copy link
Collaborator

Just thinking loud....
Do we still need copy(), as a frequency object should be immutable?
I know that we intended to change the frequency object to be immutable, but I'm not sure if we checked every case.

@jhillairet
Copy link
Member

Just thinking loud.... Do we still need copy(), as a frequency object should be immutable? I know that we intended to change the frequency object to be immutable, but I'm not sure if we checked every case.

You're right, was supposed to be done in #700. But it seems that changing an element of the array is still possible, as done in the test of this PR.

@eendebakpt
Copy link
Contributor Author

Just thinking loud.... Do we still need copy(), as a frequency object should be immutable? I know that we intended to change the frequency object to be immutable, but I'm not sure if we checked every case.

The Frequency object is not immutable right now. One can change the unit and also the frequency (via the Frequency.f numpy array). There are three setter methods, of which 2 are already deprecated. I created #878 to see whether we can actually make it immutable (except for the unit).

@eendebakpt
Copy link
Contributor Author

@FranzForstmayr @jhillairet I think this PR is ready. The Frequency is not immutable right now, and will not be during some deprecation time.

@FranzForstmayr
Copy link
Collaborator

Thank you, I'm fine with this change 👍🏻

@jhillairet jhillairet merged commit f9928fe into scikit-rf:master Apr 11, 2023
10 checks passed
@jhillairet
Copy link
Member

Thanks!

@jhillairet jhillairet mentioned this pull request May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants