AUTODIAL / AutoEIS

A tool for automated extraction of equivalent circuit models (ECM) from electrochemical impedance spectroscopy (EIS) data
https://autodial.github.io/AutoEIS/
MIT License
33 stars 7 forks source link

`preprocess_impedance_data` breaks if data contains no high frequency measurements #122

Closed vyrjana closed 4 months ago

vyrjana commented 4 months ago

Brief description and conditions for reproducing

Note that this bug is present in the latest commit on the main branch as I installed AutoEIS by cloning this repository instead of installing the latest release (0.0.29) that is available on PyPI.

I found a bug in the preprocess_impedance_data function while testing out AutoEIS. The high_freq variable (a boolean array) that is defined on the following line

https://github.com/AUTODIAL/AutoEIS/blob/603378b0e72e74dfac552a8154cbfbbe3c9e62e4/src/autoeis/core.py#L170

will not contain any True values when freq does not contain any values greater than the value of high_freq_threshold.

So, if one attempts to preprocess an impedance spectrum that spans, e.g., a frequency range from 100 mHz to 500 Hz while high_freq_threshold=1000, then a ValueError: attempt to get argmin of an empty sequency exception will be raised by the next line

https://github.com/AUTODIAL/AutoEIS/blob/603378b0e72e74dfac552a8154cbfbbe3c9e62e4/src/autoeis/core.py#L171

since Z.imag[high_freq] will be an array with a length equal to zero (since high_freq will be an array that only contains False values).

Suggested fix

This bug can be fixed by simply adding an if high_freq.any(): after line 170 and indenting the lines that are currently lines 171-174. So the following lines of code would go from this

https://github.com/AUTODIAL/AutoEIS/blob/603378b0e72e74dfac552a8154cbfbbe3c9e62e4/src/autoeis/core.py#L170-L174

to this

high_freq = freq > high_freq_threshold
if high_freq.any():
    idx_highest_valid_freq = np.argmin(np.abs(Z.imag[high_freq]))
    # Filter out frequencies above the highest valid frequency (only works if freq is sorted)
    freq = freq[idx_highest_valid_freq:]
    Z = Z[idx_highest_valid_freq:]
vyrjana commented 4 months ago

I checked out the 0.0.29 tag, deleted the old Python virtual environment, created a new virtual environment (Python 3.12.4 on Linux since I didn't mention it in the original message), and installed version 0.0.29. This version is also affected by this bug.

However, after fixing that bug I then ran into KeyError: 'MCMC' exceptions upon reaching this line both when using my own impedance spectrum and when using the one returned by io.load_test_dataset.

I noticed that pyproject.toml does not limit the versions of most of the dependencies. So, I decided to try to run the included tests locally and all except one of them passed. The one that didn't pass was the one for perform_full_analysis that was set to be skipped.

I also tried switching back to the latest commit and got the same KeyError: 'MCMC' exceptions upon reaching this line.

Have you by any chance been using the same Python (virtual) environment for a while now? Could one of the updated dependencies have made a breaking change between the release of version 0.0.29 and now? If the perform_full_analysis function has been working all this time within your development environment, then this second issue might simply have gone unnoticed due to the function never being tested in the GitHub CI in a fresh environment.

ma-sadeghi commented 4 months ago

Sorry for the late reply. For some reason, this completely dropped off my radar!

Anyway, thanks for the bug report. This is now fixed in #124, and merged into main. There are other fixes/minor improvements on main, so I'll issue a new release in a few hours.

RE the missing key issue: We've been rethinking the API a little bit, so there might be some more breaking changes pushed in the near future, but we'll try to settle on a stable API soon. The main branch should be functioning okay now.

RE perform_full_analysis, that's always been a pain to maintain as it does too much, although that's ultimately what many users want. We've deprecated the function for now (not sure if that's the right word, but basically calling it raises a NotImplementedException with a proper message), but we'll get back to it once we're happy with the API.

If you have any suggestions/feature requests, feel free to share in the issue tracker/discussion forum :)