-
Notifications
You must be signed in to change notification settings - Fork 7
Parstat input #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thanks for the pull request! I'll have a closer look at it as soon as I can. Yes, the failing test is unrelated to your changes. The issue should be resolved in an upcoming update. You can run just the tests relevant to your changes using the following command from within the
|
Yes it passes those tests no problem. Results below. python -m unittest test_data.py -v -f Ran 31 tests in 0.365s OK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some questions regarding the file format and how it is being parsed. See the comments below regarding specific lines of code in data/formats/par.py
.
src/pyimpspec/data/formats/par.py
Outdated
# Need to make sure we have the correct Action for the EIS experiment | ||
found_eis = False | ||
eis_action_no = None | ||
while not found_eis: | ||
line: str = lines.pop(0) | ||
if line.startswith("<action"): | ||
eis_action_no = str(int(line[7:][:-1]) - 1) # Parstat's action segment numbering is off by one for some reason. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with VersaStudio or the .par file format, but it seems like there can be multiple measurements. Can these measurements be, e.g., OCP -> CV -> EIS? Or just repeats of the same measurement type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, multiple experiments can be contained in a single file and they will show up in chronological order in the output file. This portion of the function will continue through the experiment setup information ("actions") to make sure it can actually find an EIS action in the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a scenario where a single file contains results from multiple EIS experiments? It might be the case that only one Segment
can be parsed at the moment.
It would be great if you could confirm that the current principle of the approach to determining whether or not a line should be parsed (i.e., line 70) would also work when there are multiple EIS experiments to parse. Your comment on line 59 states that the numbering is off by one. In the example data file that you included, the value of X
in <ActionX>
and <SegmentX>
match (in this case 1
) for the EIS experiment but the values in the Segment #
column inside the <Segment1>
section do seem to be decremented by one. I say seem, because there is also an <Action0>
section that is missing a corresponding <Segment0>
section. Also, note that there is a Segments=1
line in the <Action1>
section while the corresponding line in the <Action0>
section is Segments=0
. So the values in the Segment #
may be referring to another set of indices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid the way VersaStudio works is it allows an arbitrary number of segments based on how the user sets it up. Unfortunately, they aren't always defined the same way in the <ActionX>
sections. I have files from experiments I've run that combine open-circuit measurements over time, then EIS, then some polarization testing. The corresponding <ActionX>
sections only seem to have the Segments=X
statement for the EIS and for the initial <Action0>
block. And to make it worse, some LPR experiments have 3 different <Segment>
blocks that are just incrementally numbered based on the chronological order they're run. I can think about the best way to pull out multiple EIS data sets from a file for pyimpspec integration. It may be just as simple as counting the number of EIS <Action>
blocks in the file and making sure the correct number of <Segment>
sections are parsed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a copy of a relatively recent manual for VersaStudio and it states that the EIS data stored in the raw data files is uncalibrated and should not be used for data analysis. The data should first be opened in VersaStudio and exported (e.g., to ZView or CSV) prior to analysis.
EDIT: I don't know what VersaStudio is doing to make corrections. I hope that it doesn't rely on, e.g., calibration data that is present only on the computer that is connected to the instrument that was used to perform the measurements. That would be problematic if one had to go back and export the data after a long period of time during which the instrument might have been recalibrated, or if one tried to export the data by opening the file on another computer.
Seems strange that the corrected data isn't what is stored in the files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree it is strange. I don't particularly care for VersaStudio, though the potentiostat itself is fantastic. My general work flow is to export the data to ASCII format, then use pyimpspec for analysis. The exported data still (so far) have a .par extension. The raw data files are in a binary format that isn't easily opened, though they still have a .par extension. which is not great in my opinion.
We have had issues with the "calibration correction" that VersaStudio does in the past. For example, I've performed an anodic scan up to, say +1V vs OCP, but when I go to analyze the data the curve goes up to +15 V. So I have to go back, re-open the data in VersaStudio, and export again. It can be somewhat frustrating if you don't notice the issue in VersaStudio. To your point, if it applies new calibrations to old data, then that will of course be problematic as well.
src/pyimpspec/data/formats/par.py
Outdated
line: str = lines.pop(0) | ||
if line.startswith("<action"): | ||
eis_action_no = str(int(line[7:][:-1]) - 1) # Parstat's action segment numbering is off by one for some reason. | ||
if "potentiostatic eis" in line: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about galvanostatic EIS?
Do you know if the column headers found in the Segment
sections are the same across different measurement types? It might not be necessary to parse each Action
section if the contents of a Segment
section is enough to determine whether or not the results correspond to a potentiostatic/galvanostatic EIS measurement.
For example, it might be enough to check if the Z Real
and Z Imag
columns exist and that their corresponding values are not all zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good thought. My personal analysis functions use other information from the action sections, which may have sample area, reference electrode info, etc. I'll do a little further investigation here, it may be sufficient for pyimpspec purposes to simply look for non-zero values in those columns.
src/pyimpspec/data/formats/par.py
Outdated
assert ( | ||
len(values) >= 5 | ||
), f"Expected to parse at least five values from line: {line}" | ||
freq.append(values[9]) | ||
real.append(values[14]) | ||
imag.append(values[15]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assert is only checking that there are at least five values in the list but lines 78-80 attempt to access values with indices that might be outside of the bounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertion is just to make sure it didn't grab the wrong line. I don't recall my exact reasoning for checking that more than 5 values were present, but so far it has worked just fine for me.
|
||
def parse_par(path: str) -> List[DataSet]: | ||
""" | ||
Parse a Parstat .par file containing an impedance spectrum. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the .par file format specific to VersaStudio?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, yes.
Sorry for the long delay, I've been very busy with work. I just updated the function to be more flexible (hopefully). It seems to work just fine for the data I analyze, though I generally collect data the same way. I'm relying on the |
I'm not ready to accept this pull request at the moment. Adding support for .par files without knowing either a) how to make the necessary corrections hinted at in the manual or b) how to tell if corrections are even needed is likely to cause issues for the users down the line. If .par files are not supported, then that would hopefully force most users to first use VersaStudio to export the data into another format after corrections have been applied. I haven't had time to really look into this issue further, but I do have copies of a couple of .par files as well as the corresponding CSV and .z files exported using VersaStudio. The data in the .par files do indeed differ from the data in the CSV and .z files. The differences are most obvious at the high-frequency ends of the spectra, but there are also differences throughout the frequency ranges of those spectra. |
Understood. Do you know how the .par file you have was created? I have used exported to .csv and to ASCII, which makes the raedable .par files, and have seen no issues with the data itself. I can double check on my end. Just to re-iterate, the raw data files are not able to be read by anything other than VersaStudio, from my experience. You must export the data to another format first. It's unfortunate that the extension remains the same for ASCII files, but they are not the raw data files. |
The .par files ( I had a quick look at the differences between the data in the .par and the CSV files (i.e., by subtracting one from the other) and you can see the results in the two figures below (
|
Well I can't speak to those files because I don't really know their provenance or purpose. I ran a check of a Gamry dummy cell I use for checking calibrations, cables, etc. and obtained the results below. Fitted to a Randles cell, I obtain R1=202 Ohms, R2=2.99 kiloOhms, and C=0.967e-6 F. All of these values are within the spec limits for the cell itself. |
Is that data stored in the .par file after exporting as ASCII? Have you tried exporting the data as CSV from VersaStudio, loading it into DearEIS, and subtracting it from the data loaded from the .par file? Do you get something similar to what I posted (i.e., broadly resembling a series RL circuit)? |
…ell data collected from a parstat.
Added functions to enable parsing Parstat ASCII (.par) files, including an associated unit test.
My run_test.sh script is failing the 'test_mu' for some reason. My changes to parse Parstat files did not touch anything associated with the K-K analysis. This may be a separate bug?