fix(near equals power solver): Invalid behavior for limits#3447
fix(near equals power solver): Invalid behavior for limits#3447parapluplu wants to merge 1 commit intoOpenEMS:developfrom
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #3447 +/- ##
=============================================
+ Coverage 59.62% 59.63% +0.01%
Complexity 112 112
=============================================
Files 2894 2894
Lines 124658 124644 -14
Branches 9343 9342 -1
=============================================
- Hits 74318 74313 -5
+ Misses 47522 47520 -2
+ Partials 2818 2811 -7 🚀 New features to boost your workflow:
|
This fixes an error where a charge limit was set for a single ess inside a cluster. The power solver disrespected this, and set a value outside of the limit. For details see unit test part `OpenEMS#4` of `testNearEqualDistribution in io.openems.edge.ess.core/test/io/openems/edge/ess/core/power/PowerComponentTest.java. Without this fix, this test fails and charges higher than the allowed -1900. Testing: Unit tests, also tested in a setup with a power plant with 12 ESS in total stacked in two layers of ESS clusters. First layer is an cluster of 3 Edge2Edge ESS, second layer is a Cluster of 4 EssGenericManagedSymmetric each.
e1cb6f3 to
4c5a3f1
Compare
|
Could somebody explain to me we codecov/patch is failing with a 50% diff target? I've basically removed code and added tests. The contrary should happen and 50% is waaaaay out of line |
TBH i never understood codecov here :D |
|
I'd like to publish another merge request, that improves the null handling of the power solver in general. However, as it builds on top of this change, I would like to make my life easier and merge this one before publishing the other one. |
|
This was merged 7 hours ago with the back port merge and creates conflicts for this pull request. So if this merge request does not get any further traction I will drop this topic and will keep the changes and follow up changes in our internal repo since this now creates significant overhead. I've also changed the null handling to optionals, which makes it much more explicit and enforces correct handling whenever touching the values. Seems like Fenecon decided to go for a approach with using a constant instead of Optionals https://github.com/OpenEMS/openems/blob/develop/io.openems.edge.ess.core/src/io/openems/edge/ess/core/power/optimizers/KeepAllNearEqual.java#L27 I would prefer a more modern approach provided by java |
This fixes an error where a charge limit was set for a single ess inside a cluster. The power solver disrespected this, and set a value outside of the limit. For details see unit test part
#4of `testNearEqualDistribution in io.openems.edge.ess.core/test/io/openems/edge/ess/core/power/PowerComponentTest.java. Without this fix, this test fails and charges higher than the allowed -1900.Testing: Unit tests, also tested in a setup with a power plant with 12 ESS in total stacked in two layers of ESS clusters. First layer is an cluster of 3 Edge2Edge ESS, second layer is a Cluster of 4 EssGenericManagedSymmetric each.