Feature/mod battery cycle only

[lightningclaw001]

-added pure cycling features in a modular format
-added tests for full and half cells to test cycling

@d-cogswell @loostrum the new cycling tests are passing on my computer, but not remotely. Any thoughts?

[loostrum]

I'm not sure what's wrong with test030, that one's fine on my PC as well. Perhaps something with the noise? I remember that was an issue earlier for some tests.
test029 is also failing for me, specifically it's complaining that the shapes of the data in the reference output and new output aren't the same: I see ValueError: operands could not be broadcast together with shapes (1,40) (1,38) on the Github page. I ran it on my PC too, and I added a line to print which variable is giving problems. I get Fail from ValueError in variable phi_lyte_c: operands could not be broadcast together with shapes (47,1) (45,1) . Interestingly this is not the same exact error, but it's also a problem with mismatching shapes with an offset of 2.
I'm surprised it's working for you. Could it be you updated the reference solution for test029, but didn't commit it to the repository yet?

[lightningclaw001]

I'm not sure what's wrong with test030, that one's fine on my PC as well. Perhaps something with the noise? I remember that was an issue earlier for some tests. test029 is also failing for me, specifically it's complaining that the shapes of the data in the reference output and new output aren't the same: I see ValueError: operands could not be broadcast together with shapes (1,40) (1,38) on the Github page. I ran it on my PC too, and I added a line to print which variable is giving problems. I get Fail from ValueError in variable phi_lyte_c: operands could not be broadcast together with shapes (47,1) (45,1) . Interestingly this is not the same exact error, but it's also a problem with mismatching shapes with an offset of 2. I'm surprised it's working for you. Could it be you updated the reference solution for test029, but didn't commit it to the repository yet?

@loostrum interesting that it isn't working for you. I did push the new solutions. I don't think there is a test030, are you sure you're on the right branch? I did previously get these errors, but I realized that they were correlated with the phase separating models and that if I switched to the OCV models all my tests were passing. I figured the issue was something else and that I would tell Dan in person (and was likely that it was correlated with the noise). Are you sure you're on the latest commit of this branch? Are the config files in test025 and test029 from LIONSIMBA files?

[loostrum]

I was wrong about test030, the output actually listed Dirs030 which I assumed belonged to a test030, but that isn't the case. I double-checked that I'm on the correct branch. Indeed the config files are from LIONSIMBA. I ran the tests on both my Mac and on Linux in a docker container. On linux, only test025 fails. On mac, test025 as well as test029 fail.

On linux, the test025 error is a ValueError about shapes, but it's not the same variable that's failing every time which is a bit worrisome. It only shows the first failure, but unless the order in which the variables are checked is random I'd really expect the same failure every time. I've seen two different errors so far: a (1,47) vs (1,45) shape mismatch in partTrodecvol0part0_cbar, and a (47,11) vs (45,11) mismatch in partTrodecvol0part0_c. Do you know where the 45/47 number comes from? It seems that the reference solution has 47, while the newly calculated solution has 45.

[lightningclaw001]

I was wrong about test030, the output actually listed Dirs030 which I assumed belonged to a test030, but that isn't the case. I double-checked that I'm on the correct branch. Indeed the config files are from LIONSIMBA. I ran the tests on both my Mac and on Linux in a docker container. On linux, only test025 fails. On mac, test025 as well as test029 fail.

On linux, the test025 error is a ValueError about shapes, but it's not the same variable that's failing every time which is a bit worrisome. It only shows the first failure, but unless the order in which the variables are checked is random I'd really expect the same failure every time. I've seen two different errors so far: a (1,47) vs (1,45) shape mismatch in partTrodecvol0part0_cbar, and a (47,11) vs (45,11) mismatch in partTrodecvol0part0_c. Do you know where the 45/47 number comes from? It seems that the reference solution has 47, while the newly calculated solution has 45.

I did some digging and it seems like the reason "extra steps" are being taken is probably a tolerance issue. In some of the simulations, the (ffrac-cutoff) hits exactly zero, while the cutoff that I set was (ffrac-cutoff) >= 0. Let me see if there's some way to make the condition more tolerant of these kinds of issues.

[lightningclaw001]

@loostrum I'm going to email you a patch (since it is a little large since the test case solutions also changed), can you try testing that and see if it still fails for the tests? Thanks!

[loostrum]

HI @lightningclaw001, I tried applying the patch you sent me, but unfortunately git diff doesn't handle the difference in binary files, so it fails to apply the changes you made to the output_data.mat and input_dict files in the ref_outputs folder. Perhaps you could send me the entire repo as you have it now, or push it to a separate branch?

[lightningclaw001]

@loostrum I pushed the commits into feature/mod_battery_cycle_only_test, can you try them here? Thanks!

[loostrum]

Thanks! Unfortunately tests 25 and 29 still fail for me on the new branch, still with the 2 mismatch in shapes.

[lightningclaw001]

Thanks! Unfortunately tests 25 and 29 still fail for me on the new branch, still with the 2 mismatch in shapes.

Can you try applying this commit to that branch:
test_diff.txt
and let me know if it works? If not, can you send me the output_data.mat files that are failing?
Really appreciate it!

[loostrum]

I tried the patch, but it's not working. I'll send you the mat files in an email!

[lightningclaw001]

@loostrum I realized that multiplying the cutoff (eg. x > cutoff turns into x1e4 > cutoff1e4) by 1e4 works by turning the numbers closer to integer values but this is a very sketchy solution... do you have any other suggestions as to what to do to avoid these floating point errors?

[lightningclaw001]

also @loostrum what version of python were you using for the tests? I realized that mine fail for 3.9 but pass for 3.7 locally...

[loostrum]

I'm using python 3.6.
Not sure what we could do about the cutoff floating-point error, I'll think about it.

[d-cogswell]

I wonder if it has to do with the port class you added. Are all of the port variables being initialized? Residual equations that copy one variable to another isn't great either. Do you really need the port class?

[lightningclaw001]

I wonder if it has to do with the port class you added. Are all of the port variables being initialized? Residual equations that copy one variable to another isn't great either. Do you really need the port class?

If we want to switch to a different module but also import the data a level above I think we need to keep the port class. Pretty annoying but the other option is to go back to a nonmodular format.

I also don't think we had issues with the ports in 3.7?

[lightningclaw001]

I'm using python 3.6. Not sure what we could do about the cutoff floating-point error, I'll think about it.

So your 3.6 is also having issues with the cutoffs? Hm... weird...

[d-cogswell]

I made some progress removing the ports and residual equations that copy one variable to another. I think ports are really only for when different models need to change the same variable.

Tests now pass in different versions of Python on Linux, but fail on Windows. Will keep working on it.

[d-cogswell]

This last commit e4fc3f8 seems to have broken plotting for me?

[d-cogswell]

This check causes a crash.

[lightningclaw001]

@d-cogswell I just removed the last commit--it's no longer a dict after the old commands so some of the commands I was using wasn't working.

[d-cogswell]

@lightningclaw001 I don't think we need ab6b66c, this was already fixed in v0.1.9. Also, @v1kko is looking into why the regression tests are suddenly taking so long.

[d-cogswell]

@lightningclaw001 what do you think of this?
https://github.com/d-cogswell/mpet/commits/feature/mod_battery_cycle_only

I did some cleaning.

[lightningclaw001]

@lightningclaw001 I don't think we need ab6b66c, this was already fixed in v0.1.9. Also, @v1kko is looking into why the regression tests are suddenly taking so long.

ah yes, I remember now

[lightningclaw001]

@lightningclaw001 what do you think of this? https://github.com/d-cogswell/mpet/commits/feature/mod_battery_cycle_only

I did some cleaning.

I think other than getting rid of the tramp for the increment cycle (which helps with abrupt steps because I've had problems with them before), I'm happy with the changes

[d-cogswell]

@lightningclaw001 what do you think of this? https://github.com/d-cogswell/mpet/commits/feature/mod_battery_cycle_only
I did some cleaning.

I think other than getting rid of the tramp for the increment cycle (which helps with abrupt steps because I've had problems with them before), I'm happy with the changes

Ok, I'll restore tramp. I think I have a better solution for handling transitions, but let's save that for later.

[lightningclaw001]

@d-cogswell I'm happy with the changes now