Biologic mpr reader

[ScottSoren]

This PR adds a biologic mpr reader which makes use of the eclabfiles package. It solves #132
See NEXT_CHANGES for more details.

If you have some .mpr files at hand, it would be great if you could test it! (And add the test files on Dropbox under ixdat_resources/test_data/biologic_mpr). I would especially like a set of .mpr files from a biologic experiment with a loop to see if they append correctly.

I have only tried a few very old .mpr files some of which give errors, but want to reproduce it on newer files before raising an issue on https://github.com/vetschn/eclabfiles

I ended up doing a kind of big refactor of the biologic module, splitting it up to a base class, an mpt class, and an mpr class. Feel free to share thoughts on that structure.

The refactor led to an unexpected error in the append_ec_files demo script, which after much head-scratching I figured out had to do with a subtle bug in Measurement.__getitem__, fixed with this two-character edit: c706f09

[AnnaWiniwarter]

Your changes look ok to me, except for some typos. Of course, I don't quite understand what most parts of the code do and what they are for, so take this with a grain of salt.

However, I also tested the code on a dataset I had at hand (recorded with EClab version 11.43 which was released 2yrs ago, so it's not terribly old), and that didnt go that well. It fails with a KeyError when trying to import a CVA file. And if I just import the CP files recorded in between and add them using the + operator, the loop number stays zero. (see figures below)
I can share the datafiles offline (they are not mine), but this seems to be a more general problem of the package we rely on, I'm afraid.

So I suppose this PR has to wait until this is fixed, or the documentation updated that the MPR reader can only read CP data as of now.

[ScottSoren]

Thanks, @AnnaWiniwarter , for taking a look and for trying on a newer dataset than I had at hand! It is the same error that I got while reading a CVA mpr file (from 2017), so seems not to be the EC-Lab version. I'll raise an issue with them and also check whether eclabfiles.process() is more robust (example their readme), even though its output unfortunately requires more tedious repackaging than would a df.

[matenestor]

The eclabfiles package is "no longer being maintained" and they are referring to yadg package and its electrochem parser, so we will probably need to rewrite this.

They are even describing the binary structure of the MPR files.

[ScottSoren]

This works now! It now uses the galvani package as suggested by vetchn here: vetschn/eclabfiles#15. (yadg also looks great but seems to also not yet support CVA and CA.)
Ixdat raises a warning each time the MPR reader is used, however, encouraging the user to read .mpt's instead (because then we can debug it if needed).

Let me know if it looks good / works for you and then we can merge it in :)

[AnnaWiniwarter]

It doesn't seem to be working for me. It can read CVA files now (so that's progress), but it doesn't assign a loop number correctly (UserWarning: No 'loop number' found in ECMeasurement). "Ns" exists, but is zero for all CVs.
Unfortunately, it breaks when reading the CP files I have:

Not sure what this means, but I don't think it's ready to merge yet.

[KennethNielsen]

We can use neither galvani nor eclabfiles directly in ixdat for licensing reasons, since GPL3 is incompatible with MIT. I don't know if there is a way around it.

[ScottSoren]

@KennethNielsen I'm pretty sure this would count as what is called "dynamic linking".
It is unclear whether that would violate the GPL license: https://en.wikipedia.org/wiki/GNU_General_Public_License#Linking_and_derived_works.

It might be an important distinction that, the way it is implemented, galvani does not become a dependency of ixdat. It is not installed with ixdat, not packaged with ixdat, and only imported when/if used:

ixdat/src/ixdat/readers/biologic.py

Line 433 in bcb4867

from galvani import BioLogic

.

[ScottSoren]

Just saw that the license discussion is taking place under #152 .

[ScottSoren]

Now it should work.
When given a .mpr file, BiologicReader.read now tries both mpr-reading packages galvani and then eclabfiles. It seems galvani works for LSV, CVA, and CA, whereas eclabfiles works for OCV and CP. (I have not tried on other EC techniques)

I now get this figure from appending the .mpr files here, as in the top-level script demo_biologic_mpr_reader.py.

@AnnaWiniwarter , hopefully it works for you as well now!

With regards to loop number - I think that's a sacrifice that has to be accepted when reading .mpr's instead of .mpt's. I've added a bit to the warning that gets printed when the user tries to read a .mpr, so that it specifically says that for loop number they need to read .mpt's instead. However, cycle/step numbers seem to be there at least some of the time, and ixdat still gets the file number right when appending. As a result, there's still a robust selector, as can be seen by the plot with J_name="selector".

@matenestor , I put all the biologic reading back into one class, who's read method calls a different helper method depending on the file type. Most of the attributes of the reader only get filled out by .mpt, but I think this is the way to minimize boiler plate code and maximize simplicity of implementation.

With regards to the license: lets keep the conversation #152. What's in this PR will help many present ixdat users without any problem, so I'd like to get it merged!

[AnnaWiniwarter]

Sweet! The reader now works for all types of mpr files I have at hand (OCV, CP, CVA, GCPL (a battery technique)) and at least the "selector" returns some useful information about the different sequences.

[matenestor]

When I have installed eclabfiles only, and eclabfiles with galvani, then reading .mpr files works good for me. I have tried it just with a small dataset quickly produced in our lab, though.

So reading it is fine, but I would like some changes for the case, when neither of the packages is installed. When a user doesn't have neither galvani, nor eclabfiles installed, then they get a wall of text of exceptions. So I suggest to restructure the try blocks and modify the exception messages. I think many users will not bother to read all of that, so we could provide it in a more and nice compact form.

[matenestor]

One tiny comment, otherwise it is good to go once the conflicts are solved.

[ScottSoren]

Thanks @AnnaWiniwarter and @matenestor for the multiple reviews here!
Merged :)