Ssc 565 fix voltage discharge#568
Conversation
…oncerts about how battery_t::calculate_max_discharge_kw is dealing with q and qmax
…voltage_discharge
dguittet
left a comment
There was a problem hiding this comment.
Code looks correct, and thanks for the tests, but there are some failing tests.
|
Interesting, those pass on Windows. I have a hunch that std::sort is giving us different results on Linux and Windows. Thoughts on transitioning to std::stable_sort https://stackoverflow.com/questions/23985891/what-is-the-difference-between-stdsort-and-stdstable-sort/23986246 ? That was a recommended fix here: https://jvet.hhi.fraunhofer.de/trac/vvc/ticket/1467 and may solve some of the other issues I've been seeing (such as 10ff44b) |
|
Good news - this turned out to be a "I was having trouble building" problem instead of a "Windows vs Linux" problem. Bad news - This has turned into a cascade of failing tests. I fixed the exceptions by lowering vnom, and updated the values in voltage_t where it made sense. The core of the issue is that battery_t::calculate_current_for_power_kw does not enforce SOC limits the same way as calculate_max_discharge_kw and calculate_max_charge_kw. This is shown in DischargeBatteryModelSubHourlyWSOCLimits, and the new values in voltage_table_lib_battery_voltage_test::calculateMaxDischargeHourly_table. In the latter test, the final current value should be ~9, and the final SOC ~5 - but the way the SOC limits are applied in calculate_max_discharge_kw skew how that function reads the voltage table. If I remove the SOC limits from that function, it affects things like lib_battery_test::AdaptiveTimestepNMC, which is a pretty significant scope creep. Do you think it's possible that the SOC limits are causing problems in all of the models and that things need to be updated that significantly? |
…t capitalization.
cpaulgilman
left a comment
There was a problem hiding this comment.
This looks good for the example given in #565.
I made a couple of small revisions to the error messages. I also added an explanation in Help for 2020.11.29 Patch 2.
Is the cell nominal voltage required to be within the range shown in the UI voltage curve graph for the electrochemical model?
…ilience test but breaks a few others. Does not fix slightly unexpected results in lib_battery_voltage_test
…EL/ssc into ssc_565_fix_voltage_discharge
|
@cpaulgilman There isn't a check in the UI voltage curve graph for the voltage table's nominal voltage being included. Currently the user will be notified only after trying to simulate. |
|
@dguittet My question was about the electrochemical model, whether SSC should throw an exception if the cell nominal voltage is outside of the range defined by the electrochemical properties inputs like fully charged cell voltage, exponential cell voltage and nominal zone cell voltage. |
|
@cpaulgilman Ah okay, yes that's satisfied with https://github.com/NREL/ssc/blob/2fda6d627a400f0b9185bcb8057661004affdd70/shared/lib_battery_voltage.cpp#L269 |
dguittet
left a comment
There was a problem hiding this comment.
Looks good! travis tests should pass soon
Fix the ssc side of #565 - SSC will now throw an exception if the nominal voltage is outside of the values provided in the voltage table. Additionally add the depth of discharge guardrails to voltage_table_t::calculate_max_discharge_w - this reduces the over-dispatch issue significantly (see screenshot below) but does not solve the SOC issue reported in 565. Another issue will be created to handle that in a future patch.
New behavior:
Improvement in SOC and dispatch relative to the filed issue. Still need to fully restrict dispatch from exceeding SOC limit with a new function.