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

Vector Fitting: Custom pole initialization #845

Merged
merged 13 commits into from
Feb 18, 2023

Conversation

Vinc0110
Copy link
Collaborator

As requested in the mailing list, this PR adds an option for custom pole spacing by the user.

The pole spacing is a delicate part of the fitting process and this option is supposed to be used by advanced users only.

The goal was to keep the API unchanged, and I hope this is an elegant and usable way to do it.

@andree-sc
Copy link

Hey, thanks a lot, looks fine, except for one small thing I noticed: in line 187ff. I think a check should be made whether or not the instance variable "self.poles" holds any poles... I don't think it's that relevant, but it's the only thing I noticed now.
Other than that, nice work. I think it's an elegant solution and it doesn't change anything about how the API works thumbsup

Copy link
Member

@jhillairet jhillairet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Would like it to be included in the next release ?

@Vinc0110 Vinc0110 marked this pull request as draft February 14, 2023 18:09
@Vinc0110
Copy link
Collaborator Author

Yes! I'm currently working on the last few tweaks. Almost done...

@Vinc0110
Copy link
Collaborator Author

Vinc0110 commented Feb 14, 2023

I don't understand why the tests are failing.
@FranzForstmayr Any idea? Is it because of that decorator to filter warnings?

https://docs.pytest.org/en/6.2.x/warnings.html#pytest-mark-filterwarnings

This was referenced Feb 15, 2023
@Vinc0110 Vinc0110 marked this pull request as ready for review February 15, 2023 00:35
@Vinc0110
Copy link
Collaborator Author

Vinc0110 commented Feb 15, 2023

Ok now. I moved the tests to a separate PR (#850). This one is ready.

@Vinc0110
Copy link
Collaborator Author

@andree-sc: Thanks for the review! I added the parameter check.

@Vinc0110 Vinc0110 merged commit 185f1fc into scikit-rf:master Feb 18, 2023
@Vinc0110 Vinc0110 deleted the vf_custompoles branch February 18, 2023 14:07
@jhillairet jhillairet mentioned this pull request Feb 19, 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

3 participants