basic EC features by ScottSoren · Pull Request #6 · ixdat/ixdat

ScottSoren · 2021-01-28T14:09:34Z

Here I've flushed out the ECMeasurement class and made a CyclicVoltammagram class that adds a few features. The main focus of this PR is on how to make sure the essential data series (potential, current, and selector in this case, see class docstring to ECMeasurement) are always available by a standard name, even though they might be saved differently in different files or not present in all appended files. It proved slightly hair-pulling, even with the relative strengths of ixdat's structure.
The "front-panel" (user-facing) scripts I've been developing this with are development_scripts/ec_tools_dev.py and development_scripts/cv_tools_dev.py. They show how easy it is to manipulate and visualize a complex looped experiment with OCP, CVA and CP (ask me for the data, shared some of ec_tools_dev.py's plots in Slack yesterday).
The inside is a bit of a mess - I've used slightly different approaches to build and cache different variables, when it would be much better to have a general approach. One of @KennethNielsen 's ideas (see Slack) was to make it the Reader's responsibility to rename to the standard names and make place-holders for missing essential data. My instinct is that it is more natural to give the Technique Measurement class that responsibility, and only require the Reader to provide static lists of what things are called in the files it reads, but hope we can find the best solution while discussing the code.
It would also be nice to move a lot of the code that solves general problems to a build module or something to reduce the clutter in the technique class.
There are TODO's and FIXME's all over the code with problems and (suboptimal) ideas to improve, so especially loook at those. Hope you we can find better solutions to these problems!
Thanks!

There are some fields in .ix files (saved outside of version control) which are are not back-compatible after improvements made on [ec_tools] in response to Kenneth's review of #5

kkrempl

Just a couple questions for the measurement class :)

kkrempl · 2021-02-02T14:13:13Z

+                new_series_list.append(series)
+            else:
+                t_id = (tseries.id, tseries.backend_name)
+                # FIXME: Beautiful, met my first id clash here. Local memory and loaded


Would a solution to this be using large random numbers as identifiers?

Yeah, Kenneth has made the same point. I first resonded that I want to see it become necessary first (see #1 (comment) and #1 (comment)). And here it is. But now I have a new argument which is that if we use UUID instead of the counter, we lose the readability of the id's, which I find very useful when playing with it and the DirBackend (see slides).
It could be good to have both the small-integer id and a UUID.
But the (id, backend_name) suggested here could also work for a while. I think in any case it can wait til we have an actual SQL backend to test things against. There's TODO's in the code so we won't forget :)

kkrempl · 2021-02-02T14:24:35Z

+            new_measurement = meas
+        return new_measurement
+
+    def select(self, *args, tspan=None, **kwargs):


Not sure if I understand correctly what the select_x functions exactly do. Is it to return a measurement object that contains e.g. only the signal for M2 in a given tspan?

No, the measurement it returns contains every DataSeries which has data corresponding to the time being selected for! Select just gives access to cutting and selecting in the same function.
You can say select(tspan=[100, 200], file_number=1), for example.
Ideally you should be able to say e.g., select(cycle=1, 0.5<potential<1), but that's not implemented yet. I just added a TODO for that.

Do you think something else needs to be added to the docstring or changed?

kkrempl · 2021-02-02T14:32:15Z

@@ -333,49 +531,74 @@ def __add__(self, other):


 #  ------- Now come a few module-level functions for series manipulation ---------


Is this where we would also have integration functions? (such as in Kernel.sig_area?)

Not here. Those would be bound to a class.

But I can't decide whether it should go in Measurement or in ValueSeries. In principle the latter has all it needs (the value np array and the time np array). Which do you prefer of these two (meas is a Measurement instance)?

int_M2 = meas.calc_integral("M2", tspan=[100, 200])

int_M2 = meas["M2"].calc_integral(tspan=[100, 200])

Notice that both will have tspan wrt meas.tstamp since a timeshifted ValueSeries is returned by indexing.
But why stop there? I was thinking a ValueSeries could also have a plot() of its own. But is it a mistake to make a DataSeries more than a dumb data carrier?

kkrempl · 2021-02-02T15:17:29Z

+                f" Looked for series with names in {self.raw_current_names}"
+            )
+
+    def calibrate(self, RE_vs_RHE, A_el=None):


I would change this to calibrate_reference_electrode so we avoid conflicts with other calibration functions.

Oh, fair point. Done. Renamed calibrate_RE.
An argument could be made to not having it there at all, and letting the user just set it directly, e.g., meas.RE_vs_RHE = 0.715
I left a calibrate though which can set RE_vs_RHE, A_el, and/or R_Ohm in one go. This should I guess be overwritten by e.g. ECMSMeasurement. We could have a convention where calibrate gives access to everything in need of calibration via the specific calibrate_xxx functions (evt. of the parent classes)?

kkrempl · 2021-02-02T15:26:07Z

+        """Correct for ohmic drop by providing `R_Ohm` in [Ohm]."""
+        self.R_Ohm = R_Ohm if R_Ohm is not None else self.R_Ohm
+
+    def normalize(self, A_el):


Same here, I would use normalize_current to avoid conflict whith e.g. a ec_ms object method that also normalizes the MS signals to A_el.

Done. But since all it does is set A_el, wouldn't the expected behavior of normalize in MSMeasurement be exactly the same?

kkrempl · 2021-02-03T10:08:47Z

+        self,
+        name,
+        *,
+        technique=None,


Isn't it inheriting some of these kwargs already from the init of the Measurement class? Is there a reason to have them here again?

I wrote it all here to be explicit, so that help(ECMeasurement) will give all the info. I don't know how else to ensure that. Perhaps @KennethNielsen does.

Soren implentnting a few fixes of his work where it's available to the deconvolution work. This merge mainly brings over TODO's added in response to WS3 and first review of #6.

Minor improvements after DWS3 and first #6 rev.

ScottSoren · 2021-06-30T16:55:24Z

This has been superceded by #11 . Closing.

Ms spectrum measurement

ScottSoren added 15 commits January 26, 2021 18:09

ECMeasurement properties: potential and current

a424abf

first working EC plotter

2947325

current=0 during OCP periods

b9f5ae2

file_number and selector in ECMeasurement

23ae425

Merge branch 'ec_tools' into ec_features

01eff7a

There are some fields in .ix files (saved outside of version control) which are are not back-compatible after improvements made on [ec_tools] in response to Kenneth's review of #5

tspan cutting and selection in Measurement

48381a4

ohmic drop correction, debug save&load

550ef7f

colored axes and plot_vs_potential for ec!

b0c185e

first use of (id, backend_name) combo in Measurement

0f8dffa

start CyclicVoltammagram technique

1ff49ba

CyclicVoltammagram cycle indexing and slicing

1953d9c

working exporter for EC data

da0df21

docstrings for cv.py, plotters and exporters

af60510

docstrings in ec and new Measurement methods; debug time zero-ing

ad2b64f

debugging for ipython notebook tutorial

430d2b3

kkrempl reviewed Feb 2, 2021

View reviewed changes

kkrempl reviewed Feb 3, 2021

View reviewed changes

ScottSoren added a commit that referenced this pull request Feb 5, 2021

add TODOs from design WS #3 and 1st rev of PR #6

5cf9a40

add TODOs from design WS3 and 1st rev of PR #6

953d577

ScottSoren force-pushed the ec_features branch from 5cf9a40 to 953d577 Compare February 5, 2021 21:25

ScottSoren mentioned this pull request Feb 7, 2021

Minor improvements after DWS3 and first #6 rev. #7

Merged

kkrempl added a commit that referenced this pull request Feb 15, 2021

Merge pull request #7 from ixdat/deconvolution_sbs

85da96a

Minor improvements after DWS3 and first #6 rev.

ScottSoren mentioned this pull request Feb 24, 2021

CyclicVoltammogram tools #8

Merged

ScottSoren mentioned this pull request Jun 30, 2021

Series handling #11

Merged

ScottSoren closed this Jun 30, 2021

ScottSoren deleted the ec_features branch June 30, 2021 16:59

ScottSoren pushed a commit that referenced this pull request Jan 9, 2023

Merge pull request #6 from ixdat/ms_spectrum_measurement

d639a31

Ms spectrum measurement

		@@ -333,49 +531,74 @@ def __add__(self, other):


		# ------- Now come a few module-level functions for series manipulation ---------

Uh oh!

Conversation

ScottSoren commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kkrempl left a comment

Choose a reason for hiding this comment

Uh oh!

kkrempl Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

ScottSoren Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkrempl Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

ScottSoren Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkrempl Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

ScottSoren Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkrempl Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

ScottSoren Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkrempl Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

ScottSoren Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkrempl Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

ScottSoren Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

ScottSoren commented Jun 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ScottSoren commented Jan 28, 2021 •

edited

Loading

ScottSoren Feb 5, 2021 •

edited

Loading

ScottSoren Feb 5, 2021 •

edited

Loading

ScottSoren Feb 5, 2021 •

edited

Loading

ScottSoren Feb 5, 2021 •

edited

Loading

ScottSoren Feb 5, 2021 •

edited

Loading