Skip to content

Conversation

WardLT
Copy link
Contributor

@WardLT WardLT commented May 27, 2025

We currently assume no time elapses between multiple testing files. This PR uses the timestamps available in MACCOR output files to infer the length of a rest period.

Detecting the timestamp is complicated because some MACCOR files do not list the date along with the timestamp. We can infer it from cross-referencing with the "test time" column.

I also refactored so that the logic for making the test_time consistent between files part of a super class for all cycler parsers to avoid duplication between subclasses.

Also provides a utility for verifying that the time and test_time columns are consistent.

@coveralls
Copy link

coveralls commented May 27, 2025

Pull Request Test Coverage Report for Build 15307117181

Details

  • 52 of 52 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 96.176%

Totals Coverage Status
Change from base Build 15227174371: 0.08%
Covered Lines: 1308
Relevant Lines: 1360

💛 - Coveralls

WardLT added 2 commits May 27, 2025 16:26
We were repeating ourselves, and that became a problem with complex
strategies for combining files
@WardLT WardLT marked this pull request as ready for review May 27, 2025 21:13
WardLT added 2 commits May 28, 2025 10:41
Assume that the first one is correct, infer the remainder
@WardLT
Copy link
Contributor Author

WardLT commented May 28, 2025

Having issues with the capacity computations being too large if the last measurement in a file has a non-zero current. The capacity computation logic will therefore assume a non-zero current between whenever the battery was taken offline at the end of one file and testing was resumed at the beginning of the next.
image

@victorventuri
Copy link
Contributor

Some MACCOR files do not list the date along with the timestamp.

I'm a little confused... don't we require the timestamp to be provided in UNIX format (meaning however many seconds since like 1970 or whatever?)

Also provides a utility for verifying that the 'time' and 'test_time' columns are consistent.

It seems like we just print something if that is the case. Should we raise an error, or a warning, or something a bit more severe?

I'm not sure I understand some of the other modifications, but otherwise it looks fine to me. Would recommend another pair of eyes to take a look, seeing as I really don't think I am the best person to review some of these changes...

@WardLT
Copy link
Contributor Author

WardLT commented May 30, 2025

I'm a little confused... don't we require the timestamp to be provided in UNIX format (meaning however many seconds since like 1970 or whatever?)

That's the requirement for our file formats. The MACCOR files we're reading are more relaxed. They'll either give you "DD/MM/YYYY HH:MM:SS" or just "HH:MM:SS" with the start date listed in the top. Check out https://github.com/ROVI-org/battery-data-toolkit/blob/day-rollover/tests/files/maccor_example.002

It seems like we just print something if that is the case. Should we raise an error, or a warning, or something a bit more severe?

That's right. I opted to make these "Checker" tools return a string and leave it up to the whomever uses them to decide whether it should be warning/error/exception.

@WardLT WardLT changed the title Check for day rolling over in MACCOR files Improve reading experiments across multiple files. Jun 2, 2025
@WardLT WardLT changed the title Improve reading experiments across multiple files. Improve reading experiments stored in multiple files Jun 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants