-
Notifications
You must be signed in to change notification settings - Fork 11
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
basic EC features #6
Conversation
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
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.
Just a couple questions for the measurement class :)
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 |
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.
Would a solution to this be using large random numbers as identifiers?
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.
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 :)
new_measurement = meas | ||
return new_measurement | ||
|
||
def select(self, *args, tspan=None, **kwargs): |
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.
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?
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.
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?
@@ -333,49 +531,74 @@ def __add__(self, other): | |||
|
|||
|
|||
# ------- Now come a few module-level functions for series manipulation --------- |
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 this where we would also have integration functions? (such as in Kernel.sig_area?)
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.
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?
src/ixdat/techniques/ec.py
Outdated
f" Looked for series with names in {self.raw_current_names}" | ||
) | ||
|
||
def calibrate(self, RE_vs_RHE, A_el=None): |
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 would change this to calibrate_reference_electrode so we avoid conflicts with other calibration functions.
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.
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)?
src/ixdat/techniques/ec.py
Outdated
"""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): |
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.
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.
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.
Done. But since all it does is set A_el
, wouldn't the expected behavior of normalize
in MSMeasurement
be exactly the same?
self, | ||
name, | ||
*, | ||
technique=None, |
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.
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?
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 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.
5cf9a40
to
953d577
Compare
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.
This has been superceded by #11 . Closing. |
Hey @KennethNielsen and @kkrempl ,
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 ofixdat
's structure.The "front-panel" (user-facing) scripts I've been developing this with are
development_scripts/ec_tools_dev.py
anddevelopment_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!