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

Enable multiple right-hand sides, fix C-division and n=3-edge case, made tests elaborate #26

Closed

Conversation

MothNik
Copy link

@MothNik MothNik commented Jun 8, 2024

Edit June 09, 2024
#27 is an improved version of this pull request. The following describes the basic changes that #27 relies on. However, this pull request is not really required anymore. I will still leave it open for a bit.


This pull request primarily tackles #11 by enabling multiple right hand sides for the pentapy-solvers. Currently, this is only implemented in a serial fashion because the current API does not specify the memory layout of the input Arrays. This makes it quite hard to completely remove the GIL which is required for parallelization of Cython level, but the serial fashion implements the feature in a first step
Implementing this required a shotgun refactoring on Cython-level for a consistent, scalable flow and memory handling that relies on memoisation.

Furthermore the pull request fixes

I've found the development with the current package setup causes so much friction on simple tasks and maintainability was not really facilitated, so

  • the internal handling for the solver aliases ("1", 1, "PTRANS-I", etc.), that introduced super much duplicated code, could be highly simplified by transitioning to a straightforward and safe approach based on an Enum-conversion and only relying on single checks. Also, the string-input was made case-insensitive.
  • empty lines were inserted between blocks of coherent logics to make it human- and not only machine-readable and reduce the risk for bugs by overlooking a line.
  • the Cython functions are now documented with comments and docstrings on the low level implementation details given that they are the centrepiece of the package after all.
  • extensive tests were added for the external solvers because providing this functionality untested is a bit shady, especially when they are also excluded from coverage. They don't need to be tested extensively, but automated tests whether they are even callable are the bare minimum that has to be provided.
  • tests were also added for the error handling, which is one of the key parts of the package.
  • requirements were moved (mabye back?) to files that are dynamically linked in the pyproject.toml. To install a new dependency for development, pip install -r some_requirements.txt is way simpler than pip install pentapy[...] which takes very long for rebuilding pentapy. Actually, I'm not even sure whether all the development requirements should be in the pyproject.toml.
    Nobody who just installs the package will notice the difference, but for development, this ensures a proper workflow.
    NOTE: this was done in a single commit, so it can be reverted in one go if desired.
  • error messages were enriched with debug information that the user can also work with, e.g., matrix has to be n x n is now matrix has to be n x n, got 10 x 15, so the user knows where to check.
  • pentapy.solve is now fully typed, especially the solver-argument which can be specified by so many literals that leaving it untyped reduces the joy to work with it. Now, any IDE will show all the options as one hovers over the function name and also autocompletion with proper input is now easily possible.

Finally, some typos were fixed. With a VSCode extension like CSpell or a similar spell checker, this can be avioded.

I tried to update the changelog, but feel free to adapt this. I don't really know what has to be updated in terms of other documentations and stuff around, but you know better. I also don't know where to update the version and if these changes are a minor or a major jump for the version.

I really hope that I didn't miss anything, but all the new tests pass and the bound checker never ran into an out of bounds error ✅

Sorry, that this is such a big PR, but I would not have been able to ensure that everything works fine with the current package setup 🙏

MothNik added 30 commits May 25, 2024 17:42
…proved import chain; improved code readability
…utput and warning behaviour; tested it altogether
…tate development without compromising on build
@MothNik
Copy link
Author

MothNik commented Jun 8, 2024

This Pull request should be squashed before merging.

@MothNik
Copy link
Author

MothNik commented Jun 8, 2024

For the documentation, the only thing that changed is the API of solve.
I hope you like the additional details, although the type hint for solver on top is a bit big.

@MothNik MothNik changed the title Enable multiple right-hand sides, fix C-division and n=3-edge case, made tests elaborate 11 Enable multiple right-hand sides, fix C-division and n=3-edge case, made tests elaborate Jun 9, 2024
@MothNik MothNik changed the title 11 Enable multiple right-hand sides, fix C-division and n=3-edge case, made tests elaborate Enable multiple right-hand sides, fix C-division and n=3-edge case, made tests elaborate Jun 9, 2024
@MothNik MothNik marked this pull request as draft June 10, 2024 14:21
@MothNik MothNik closed this Jul 23, 2024
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.

1 participant