Skip to content

Refactor: Modularized project into src/ layout and integrated pytest-…#2

Open
JoseFelipeFerrada wants to merge 2 commits intoSebastianL18:masterfrom
JoseFelipeFerrada:refactor/src-layout-and-benchmarks
Open

Refactor: Modularized project into src/ layout and integrated pytest-…#2
JoseFelipeFerrada wants to merge 2 commits intoSebastianL18:masterfrom
JoseFelipeFerrada:refactor/src-layout-and-benchmarks

Conversation

@JoseFelipeFerrada
Copy link

Hi!

I've been using your library for my master thesis (it's been of great help! c: ) and wanted to make little changes. Mostly use more vectorized calculations from numpy. So i created a PyTest testsuit, added some test of pole retrieval and let the existing test as benchmark of speed and accuracy.

I have been thinking, are the assert i added meaningful? perhaps i should add something more dynamic?

i also added a toml file to make it more like a python library.
Hope it helps :)

imagen

Copy link
Owner

@SebastianL18 SebastianL18 left a comment

Choose a reason for hiding this comment

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

Hi.

I am happy that the repository has been helpful for you. Thank you in advance for your ideas and contributions. The benchmark functions and files that you have created are really good, and will significantly improve this repository. I would like to add your contributions to the main branch after some discussion and feedback, because some of the implementations you have created are new for me!

I reviwed your files and leave some comments and suggestions. Additionally, I would be really grateful if you help me writhing a brief explanation about the new tests cases you created, that can be included in the README file.

I will be waiting for reply and feedback!

Best regards!

Sebastian Loaiza

Copy link
Owner

Choose a reason for hiding this comment

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

vectfit3.py module is moved to a src folder whit out code changes. Right?

Copy link
Author

Choose a reason for hiding this comment

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

i deleted scipy pi and used numpy one. no more changes. locally i tested numpy vectorization

Copy link
Owner

Choose a reason for hiding this comment

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

Good contribution. Now test cases can be executed without a local path to the DATA files?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, load_vectfit_csv fixture handles the loading via pandas. so the test is more readable

Copy link
Owner

Choose a reason for hiding this comment

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

Interesting experiments of poles and residues recovering. I would like to add them as examples for the main repository.

@@ -0,0 +1,87 @@
import numpy as np
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
import numpy as np

@@ -0,0 +1,87 @@
import numpy as np
import pytest
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
import pytest

Copy link
Owner

Choose a reason for hiding this comment

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

I found some extra imports, maybe typos when coding.

authors = [
{name = "Sebastian Loaiza Elejalde", email = "author@example.com"},
]
dependencies = [
Copy link
Owner

Choose a reason for hiding this comment

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

Missing matplotlib dependency? In fact vecfit3.py imports this lib because it is used to plot fitting results. Probably it is necessary to add matplotlib as another dependency.

Copy link
Author

Choose a reason for hiding this comment

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

oops, indeed i missed matplotlib suggestion

Copy link
Owner

Choose a reason for hiding this comment

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

What if we write a new driver code for those that want to clone and run default tests locally and inmediatly?

Copy link
Author

@JoseFelipeFerrada JoseFelipeFerrada Mar 5, 2026

Choose a reason for hiding this comment

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

like a script? pytest automatically discovers the test when on the folder must have pytest and pytest-benchmark. run the command "python -m pytest" and let the test run

Co-authored-by: Sebastian Loaiza Elejalde <144065519+SebastianL18@users.noreply.github.com>
@JoseFelipeFerrada
Copy link
Author

regarding the tests.
we have 3 files, "previous" are the test you had. they now benchmark the speed and the accuracy( via extra arguments) i let them that way because they are usecases, and is useful to measure the speed and improve them

the other two are recover and vector recover
in recover we test the convergence of the 3 asym cases
on vector we test the convergence of vector with equal weight and one with random (i had problem with this case but when creating the test on the new code it passed flawlessly did you fix them? haha)

Performance Benchmarks (test_previous.py): Wraps the original real-world use cases (transformer data, 6x6 admittance matrices, etc.) to benchmark execution time .

Scalar Recovery (test_recover.py): Generates synthetic data from ground-truth transfer functions to verify the algorithm recovers the known terms. Systematically tests asymptote configurations (asymp=1, 2, and 3) with D and E term recovery

Vector Recovery (test_vector_recover.py): Validates correctness in multi-dimensional fitting (4xN). Includes tests with both uniform and randomized weighting arrays to verify the weighting implementation functions correctly.

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