search

Solid-Energy-Systems / NewareNDA

Python module and command line tool for reading and converting Neware nda and ndax battery cycling data files.
BSD 3-Clause "New" or "Revised" License
17 stars 10 forks source link

Missing multiplier: add option to allow no-rescaling #47

Closed ml-evs closed 9 months ago

ml-evs commented 9 months ago

Hi there, thanks for this useful package! I've been trying to integrate Neware support into navani (a wrapper for several packages that allows us to normalize battery data across all brands in our lab) and thus datalab (our group's data management platform).

Some of the test data I've been provided does not work with the current version of this package, as we are missing the instrument Range=5. I would really like to be able to use this package in a mode that is robust to missing multipliers, e.g., give a gigantic warning but do not error if the instrument scaling has not been determined, as in our case, the user can supply the desired scaling manually to be stored alongside the data file. Either this could be enabled by an option, or be the default.

I've coded this up in my fork which I can continue to use for now, but thought I should open an issue in the meantime to accompany an upcoming PR. Is this functionality you would like to add?

d-cogswell commented 9 months ago

Hi @ml-evs multiplier_dict contains a mapping for each hardware range setting on Neware equipment. As more and more people use the code on files from different hardware, the dictionary is being filled in and the error occurs less frequently. I intended that a missing multiplier throw an error, so that new entries can be added to the dictionary (by comparing with BTSDA). You may only need to add 1 or 2 entries to the dictionary to get all of your files to load. Want to give this a try?

The risk with overriding the error with a default of 1 is you may unknowingly be returning incorrect current and capacity values.

ml-evs commented 9 months ago

Hi @ml-evs multiplier_dict contains a mapping for each hardware range setting on Neware equipment. As more and more people use the code on files from different hardware, the dictionary is being filled in and the error occurs less frequently. I intended that a missing multiplier throw an error, so that new entries can be added to the dictionary (by comparing with BTSDA). You may only need to add 1 or 2 entries to the dictionary to get all of your files to load. Want to give this a try?

The risk with overriding the error with a default of 1 is you may unknowingly be returning incorrect current and capacity values.

Yep, understood, if you're happy to keep up with new instruments then I'm happy to help! I still think an override would be useful but understand that it might prevent people from contributing back.

The Range I am missing has the value 5 from one of the test files over at https://github.com/FTHuld/neware_reader: https://github.com/FTHuld/neware_reader/tree/master/tests/testdata I don't have the Neware software to be able to read and give you any more info than that, unfortunately.

d-cogswell commented 9 months ago

Hi @ml-evs I just pushed a commit to development with the missing current range. The file new_nda_file.nda (nda version 29) now loads now without any errors.

The file old_nda_file.nda is older than any I have seen before (nda version 8), and NewareNDA is failing to find the start of the data section. This can be tricky to fix without a sample of similarly old files.

For reference, here is the Neware utility I use to load nda/ndax files and check that current and capacity are being scaled correctly: https://newarebattery.com/softwares/BTSDA20221226.zip

ml-evs commented 9 months ago

Much appreciated, and that link looks super useful. I'll close my PR and this issue now. Thanks!