Skip to content

Features/wrapper#5

Merged
masinag merged 22 commits into
mainfrom
features/wrapper
May 19, 2025
Merged

Features/wrapper#5
masinag merged 22 commits into
mainfrom
features/wrapper

Conversation

@SebastianoTaddei

Copy link
Copy Markdown
Contributor

Fixes #2

This is the first draft of the new python api. Documentation is still a little lacking, but it should suffice for now to evaluate it. Of course no tests for it yet, but we could use this directly in testing to test both the bindings and the interface.

@masinag give it a spin and let me know how it feels

@SebastianoTaddei SebastianoTaddei added the enhancement New feature or request label May 11, 2025

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request introduces a new Python API for B-spline evaluations and interpolations, laying out the initial wrapper classes and adjusting the build and packaging configurations.

  • Implements the BSpline and AdditionalConditions classes in the Python wrapper.
  • Updates build configurations in CMakeLists.txt, pyproject.toml, MANIFEST.in, and SWIG interface files.
  • Adjusts module exports and dependency management.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
wrapper/bsplinex/bspline.py Introduces the BSpline wrapper class with API methods for evaluation and interpolation.
wrapper/bsplinex/additional_conditions.py Implements the AdditionalConditions class with input validation.
wrapper/bsplinex/init.py Exports modules and factory methods as part of the public API.
pyproject.toml Updates package naming and dependency specifications.
MANIFEST.in Includes the new wrapper source directory.
CMakeLists.txt Adjusts SWIG build properties and installation paths.
BSplineX.i Updates SWIG configuration for binding generation.
BSplineX Updates subproject commit reference.

Comment thread wrapper/bsplinex/bspline.py Outdated
Comment thread wrapper/bsplinex/additional_conditions.py Outdated
Comment thread wrapper/bsplinex/additional_conditions.py Outdated
SebastianoTaddei and others added 3 commits May 11, 2025 10:48
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

@masinag masinag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's change at least the examples to use the new interface.

Comment thread BSplineX.i Outdated
Comment thread wrapper/bsplinex/additional_conditions.py Outdated
Comment thread wrapper/bsplinex/additional_conditions.py Outdated
Comment thread wrapper/bsplinex/bspline.py Outdated
Comment thread wrapper/bsplinex/bspline.py Outdated
Comment thread wrapper/bsplinex/bspline_factory.py Outdated
Comment thread wrapper/bsplinex/bspline_factory.py
Comment thread wrapper/bsplinex/bspline_factory.py Outdated
Comment thread wrapper/bsplinex/bspline_factory.py Outdated
Comment thread wrapper/bsplinex/bspline.py Outdated
@SebastianoTaddei

Copy link
Copy Markdown
Contributor Author

Let's change at least the examples to use the new interface.

I agree, they will come tomorrow. For now review the latest changes

Comment thread wrapper/bsplinex/interpolating_condition.py
@SebastianoTaddei

Copy link
Copy Markdown
Contributor Author

After using the new API to quickly update and improve the examples, I believe it feels much better. Let me know what you think. If you agree, I will remove the current test and move to close this feature. Afterward I will open a new one to add the converted C++ tests here as well.

Do you still like the idea of having the same C++ tests or do you prefer not to test it again and simply test the interface? The latter is probably more maintainable if we trust the swig generation to be correct

@masinag masinag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After using the new API to quickly update and improve the examples, I believe it feels much better. Let me know what you think.

Yes, much better.

If you agree, I will remove the current test and move to close this feature. Afterward I will open a new one to add the converted C++ tests here as well.

I agree.

Do you still like the idea of having the same C++ tests or do you prefer not to test it again and simply test the interface? The latter is probably more maintainable if we trust the swig generation to be correct

We can go with the latter, it's still not clear to me how to only test the interface, but I'm open to the idea.

@SebastianoTaddei

Copy link
Copy Markdown
Contributor Author

I don't know either how we should test the interface. The only thing that comes to mind is testing if the output of a vector input is a vector of the same size or something. But maybe it doesn't make sense

@masinag

masinag commented May 18, 2025

Copy link
Copy Markdown
Contributor

Ahh yes it makes sense, like the integration with numpy etc.
Anyway, we can discuss it in the future, for now we can just remove the tests

Comment thread wrapper/bsplinex/bspline.py
New ones will come in another PR
@SebastianoTaddei

Copy link
Copy Markdown
Contributor Author

I thought that no tests ran would imply passing tests, but no. Nonetheless, I think we can merge and start working on the new tests.

@masinag

masinag commented May 19, 2025

Copy link
Copy Markdown
Contributor

I think we cannot merge with failing checks. Just add a dummy test to make it happy

@SebastianoTaddei

Copy link
Copy Markdown
Contributor Author

For now I think we are fine, I will open an issue over at the C++ project to decide wether to remove the get_ from those methods as well.

@masinag masinag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀

@masinag masinag merged commit 9870ec9 into main May 19, 2025
3 checks passed
@SebastianoTaddei SebastianoTaddei deleted the features/wrapper branch May 21, 2025 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The automatically generated SWIG API sucks

3 participants