Skip to content

Conversation

@ChriKo97
Copy link

Pull Request

Description

Adds new case for not accepted orders preventing errors in set_dispatch_plan method of baseUnit

Changes Proposed

  • Currently, not accepted orders returned to unit lead to error in dispatch plan, as "accepted_price" key is not set in order object
  • New case prevents this error and sets accepted price to 0.

Testing

No testing

Checklist

Please check all applicable items:

  • Code changes are sufficiently documented (docstrings, inline comments, doc folder updates)
  • New unit tests added for new features or bug fixes
  • Existing tests pass with the changes
  • Reinforcement learning examples are operational (for DRL-related changes)
  • Code tested with both local and Docker databases
  • Code follows project style guidelines and best practices
  • Changes are backwards compatible, or deprecation notices added
  • New dependencies added to pyproject.toml
  • A note for the release notes doc/release_notes.rst of the upcoming release is included
  • Consent to release this PR's code under the GNU Affero General Public License v3.0

@codecov
Copy link

codecov bot commented Aug 13, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.31%. Comparing base (5134050) to head (58f05cf).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
assume/common/base.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #628      +/-   ##
==========================================
- Coverage   80.32%   80.31%   -0.01%     
==========================================
  Files          53       53              
  Lines        7694     7696       +2     
==========================================
+ Hits         6180     6181       +1     
- Misses       1514     1515       +1     
Flag Coverage Δ
pytest 80.31% <66.66%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kim-mskw
Copy link
Contributor

Hi @ChriKo97,

could you please explain what leads to that error? According to my current understanding, this should not happen, since even if the orders are rejected, they have an accepted price.

@ChriKo97
Copy link
Author

Hi @kim-mskw,

currently the accepted_price needs to be set in the clearing mechanism. @maurerle and I decided that it may be useful to allow to not set the accepted_price in the clearing mechanism, as it could be misleading (order is not accepted but has an accepted price). The proposed change in the pull request would allow new clearing mechanism to omit the "accepted_price" key in the returned orderbook

@kim-mskw
Copy link
Contributor

Hi @kim-mskw,

currently the accepted_price needs to be set in the clearing mechanism. @maurerle and I decided that it may be useful to allow to not set the accepted_price in the clearing mechanism, as it could be misleading (order is not accepted but has an accepted price). The proposed change in the pull request would allow new clearing mechanism to omit the "accepted_price" key in the returned orderbook

I am hesitant with this change, as some of the capabilities in Assume, namely the learning, depend on the actual accepted price being set in the clearing , always, despite the non-acceptance of bids. So this would lead to some clearing algorithms not working with some bidding strategies; of course, they do not work with each other all the time. Still, the accepted volume is 0 if nothing is accepted, why the price then misleading? Rather, change the naming instead of making extra cases for different clearing algorithms?

@maurerle
Copy link
Member

Yes true, this was the reasoning for it.

I would still keep this PR open to eventually find a better solution, which conforms with learning.
It surely is a rake laying around.
And every new user will trip into this when creating a new market mechanism :/

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.

3 participants