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

Maximize compatability with Datatypes by returning NotImplemented if __add__, __mul__ ... fail #417

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

FBumann
Copy link
Contributor

@FBumann FBumann commented Feb 14, 2025

Closes # (if applicable).

Changes proposed in this Pull Request

Arithmetic Operations with custom classes, which rely on xarray or another compatible Datatype are now able to define how they interact with linopy.Variable, linopy.LinearExpression and linopy.QuadraticExpression

Checklist

  • Code changes are sufficiently documented; i.e. new functions contain docstrings and further explanations may be given in doc.
  • Unit tests for new features were added (if applicable).
  • A note for the release notes doc/release_notes.rst of the upcoming release is included.
  • I consent to the release of this PR's code under the MIT license.

@FBumann
Copy link
Contributor Author

FBumann commented Feb 14, 2025

Fell free to remove the tests I created. But they are the best explanation I can give on what the functionality is supposed to do

@FBumann
Copy link
Contributor Author

FBumann commented Feb 18, 2025

Do I need to put some extra work in to get this PR merged like achieving full code coverage? Just let me know. I have little experience in contributing to other packages.

Copy link
Member

@lkstrp lkstrp left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @FBumann ! And welcome!

In General the CI checks should all pass. Some checks are sometimes unstable, but "Check Types" and Code Coverage should always pass. Which means extra tests are always needed, so your tests are great!

  • For Code Coverage you need to add some Tests, which checks for the NotImplemented Error as well.
  • The mypy issues you can find here or run mypy locally

Otherwise you just need to wait for a Review, which sometimes can take a couple of days.

Looks fine for me, when the tests run through. But I give @FabianHofmann the honor to merge.

@@ -4,6 +4,8 @@ Release Notes
.. Upcoming Version
.. ----------------

.. * Added support for arithmetic operations with custom classes.
Copy link
Member

Choose a reason for hiding this comment

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

.. means this is commented out, please remove it for your note and the "upcoming version" header

@FabianHofmann
Copy link
Collaborator

that's a very interesting thing! out of curiosity, how do you plan to use it? different kind of arrays like polars or so?

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.

3 participants