-
Notifications
You must be signed in to change notification settings - Fork 30
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
Added support for v11.50 : #95
Conversation
… information !! Added support for v11.50 : Few modifications in the VMPdata_dtype_from_colIDs Added new headers VMPmodule_hdr_v2 Modified MPRfile initialization
Thanks for this @JhonFlash3008! As you say the diff is quite large at the moment, I think I'll try to analyse it locally and push some tweaks so that we can see whats actually changed. |
My guess is that you saved the files with CRLF (Windows) line endings and forgot to configure git to convert the line endings to Unix style before committing. That means every line in every file is changed. |
I see in the commit message that there is also autoformat with Black. That should go in a separate commit... |
I'd also undone this (for some reason it didn't push before your comments). I think the standalone black + linting fixes PR makes sense (to avoid this exact scenario again), and then another follow-up PR at some point that enforces formatting in the CI. |
I've just tried to unpick the merge conflicts after #98, will make sure the code is unchanged during review (and then ideally we can rebase this PR a bit to make clearer what has happened). |
Just to be clear @chatcannon, this PR needs a proper review with additional test files etc. before merging, perhaps we could do an intermediate release with the latest updates and then I can spend a bit of time next week on this one? |
I prepared a test with a bunch of .mpr and .mpt files from the v11.50. Do you want me to send it ? |
Sounds great! Yes, just add the tests to the bottom of the test file with sensible names and we can tidy them up. This repo uses Git LFS for the files (more efficient way of tracking changes in big files that won't change much). This is normally available in most git distributions; before you commit the additional test files you just need to run |
So I added the tests with multiple BioLogic files as I wanted to know if it worked for various techniques (OCV, PEIS, GEIS, CP, CA, MB, GCPL). I put them in a specific folder, don't know if it's what you wanna do. |
It raised a side question for me : when we load an MPT, we modify fields < Ewe > to Ewe and < I > to I. When we load an MPR we don't. The question is, do we want to keep the info that the measurement are averaged ? If yes, then we souldn't rename the columns in the MPTfile, if not then you should rename the columns in the MPRfile to make it easier to postprocess |
I think that dates back to me not understanding what the |
Add support for EC-Lab v11.50 Rebased from #95 by @JhonFlash3008
Superseded by #97 |
Add support for EC-Lab v11.50 Rebased from echemdata#95 by @JhonFlash3008
Few modifications in the VMPdata_dtype_from_colIDs Added new headers VMPmodule_hdr_v2
Modified MPRfile initialization
Also... Lots of style editing due to autoformat with Black -> lots of useless…information !!
Need to include tests