pybamm-team / liionpack

A battery pack simulation tool that uses the PyBaMM framework
https://liionpack.readthedocs.io/en/latest/
MIT License
83 stars 26 forks source link

[Bug]: events_names shape does not match events_change shape #282

Closed mleot closed 4 months ago

mleot commented 6 months ago

liionpack Version

0.3.9

Python Version

3.10.0

Describe the bug

When simulating an experiment in liionpack, and when the voltage limits are hit, the CasadiManager attempts to go through all the events

    def log_event(self):
        event_change = np.asarray(self.actors[0].get_event_change())
        Nr, Nc = event_change.shape
        event_names = self.actors[0].get_event_names()
        for r in range(Nr):
            if np.any(event_change[r, :]):
                lp.logger.warning(
                    event_names[r]
                    + ", Batteries: "
                    + str(np.where(event_change[r, :])[0].tolist())
                )

This will often raise a list index out of range error, as I have found that there are four event names, but Nr holds a value larger than that (I have seen 17 on one example).

I am unsure how event_change is structured internally, but it appears to have too many event_names.

Steps to Reproduce

  1. Go to the JellyBaMM Repo and open the example notebook "03 Running A Simulation"
  2. Set the experiment length to something long enough where a voltage cutoff is hit.
  3. Run the simulation.

Expected behaviour

The voltage cutoff should be hit, and it should be reported that a limit has been reached. Next, it will go through three event names and list which cells hit the Maximum voltage limit, Maximum voltage limit switch, and Minimum voltage limit switch. It will try to log another event but because then length of event_names is shorter than Nr (as described above), it will raise an index out of range error.

Relevant log output

------------------------------------------------------------
WARNING    : Event: Minimum voltage switch [V], Batteries: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255, 256, 257, 258, 259, 260, 261, 262, 263, 264, 265, 266, 267, 268, 269, 270, 271, 272, 273, 274, 275, 276, 277, 278, 279, 280, 281, 282, 283, 284, 285, 286, 287, 288, 289, 290, 291, 292, 293, 294, 295, 296, 297, 298, 299, 300, 301, 302, 303, 304, 305, 306, 307, 308, 309, 310, 311, 312, 313, 314, 315, 316, 317, 318, 319, 320, 321, 322, 323, 324, 325, 326, 327, 328, 329, 330, 331, 332, 333, 334, 335, 336, 337, 338, 339, 340, 341, 342, 343, 344, 345, 346, 347, 348, 349, 350, 351, 352, 353, 354, 355, 356, 357, 358, 359, 360, 361, 362, 363, 364, 365, 366, 367, 368, 369, 370, 371, 372, 373, 374, 375, 376, 377, 378, 379, 380, 381, 382, 383, 384, 385, 386, 387, 388, 389, 390, 391, 392, 393, 394, 395, 396, 397, 398, 399, 400, 401, 402, 403, 404, 405, 406, 407, 408, 409, 410, 411, 412, 413, 414, 415, 416, 417, 418, 419, 420, 421, 422, 423, 424, 425, 426, 427, 428, 429, 430, 431, 432, 433, 434, 435, 436, 437, 438, 439, 440, 441, 442, 443, 444, 445, 446, 447, 448, 449, 450, 451, 452, 453, 454, 455, 456, 457, 458, 459, 460, 461, 462, 463, 464, 465, 466, 467, 468, 469, 470, 471, 472, 473, 474, 475, 476, 477, 478, 479, 480, 481, 482, 483, 484, 485, 486, 487, 488, 489, 490, 491, 492, 493, 494, 495, 496, 497, 498, 499, 500, 501, 502, 503, 504, 505, 506, 507, 508, 509, 510, 511, 512, 513, 514, 515, 516, 517, 518, 519, 520, 521, 522, 523, 524, 525, 526, 527, 528, 529, 530, 531, 532, 533, 534, 535, 536, 537, 538, 539, 540, 541, 542, 543, 544, 545, 546, 547, 548, 549, 550, 551, 552, 553, 554, 555, 556, 557, 558, 559, 560, 561, 562, 563, 564, 565, 566, 567, 568, 569, 570, 571, 572, 573, 574, 575, 576, 577, 578, 579, 580, 581, 582, 583, 584, 585, 586, 587, 588, 589, 590, 591, 592, 593, 594, 595, 596, 597, 598, 599, 600, 601, 602, 603, 604, 605, 606, 607, 608, 609, 610, 611, 612, 613, 614, 615, 616, 617, 618, 619, 620, 621, 622, 623, 624, 625, 626, 627, 628, 629, 630, 631, 632, 633, 634, 635, 636, 637, 638, 639, 640, 641, 642, 643, 644, 645, 646, 647, 648, 649, 650, 651, 652, 653, 654, 655, 656, 657, 658, 659, 660, 661, 662, 663, 664, 665, 666, 667, 668, 669, 670, 671, 672, 673, 674, 675, 676, 677, 678, 679, 680, 681, 682, 683, 684, 685, 686, 687, 688, 689, 690, 691, 692, 693, 694, 695] 
SOURCE     : liionpack.logger.log_event 
TIME STAMP : 2024-02-29 18:24:48,493
------------------------------------------------------------

File ~/liionpack/liionpack/liionpack/solvers.py:296, in GenericManager._step(self, step, updated_inputs)
    294     vlims_ok = False
    295 # 07 Step the electrochemical system
--> 296 self.step_actors()
    297 return vlims_ok

File ~/liionpack/liionpack/liionpack/solvers.py:497, in CasadiManager.step_actors(self)
    495 events = self.actors[0].step(self.build_inputs()[0])
    496 if events:
--> 497     self.log_event()
    498 toc = ticker.time()
    499 lp.logger.info(
    500     "Casadi actor stepped in time " + str(np.around(toc - tic, 3)) + "s"
    501 )

File ~/liionpack/liionpack/liionpack/solvers.py:527, in CasadiManager.log_event(self)
    523 for r in range(Nr):
    524     # try:
    525     if np.any(event_change[r, :]):
    526         lp.logger.warning(
--> 527             event_names[r]
    528             + ", Batteries: "
    529             + str(np.where(event_change[r, :])[0].tolist())
    530         )

IndexError: list index out of range

Additional context

From my own research, it appears to be a related to the generation of the casadi_objs variable in the cco() function call (see these lines of code ).

I don't quite understand the logic behind:

    all_vars = sorted(sim.model.variables.keys())
    event_vars = [v for v in all_vars if "Event" in v]
    if len(event_vars) > 0:

where if len(event_vars) > 0: is true, then a repeat of the creation of variables functions is performed code with the only difference being they are now named events_fn.

It may be my ignorance in understanding how the code works, but this appears to be directly related to the best of my knowledge. I would think that only n number of events_fn items would be created so that there are the same number of events_names.

TomTranter commented 4 months ago

Can you pull latest changes after #288 and see if that fixes the problem

mleot commented 4 months ago

Yes the fix that you applied was the fix that worked for me.

mleot commented 4 months ago

Yes the fix that you applied was the fix that worked for me.