Skip to content
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

Closed
wants to merge 7 commits into from

Conversation

JhonFlash3008
Copy link
Contributor

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

… information !!

Added support for v11.50 :
Few modifications in the VMPdata_dtype_from_colIDs
Added new headers VMPmodule_hdr_v2
Modified MPRfile initialization
@ml-evs
Copy link
Collaborator

ml-evs commented Jan 20, 2024

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.

@chatcannon
Copy link
Collaborator

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.

@chatcannon
Copy link
Collaborator

I see in the commit message that there is also autoformat with Black. That should go in a separate commit...

This was referenced Jan 20, 2024
@ml-evs
Copy link
Collaborator

ml-evs commented Jan 20, 2024

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'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.

@ml-evs
Copy link
Collaborator

ml-evs commented Jan 20, 2024

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).

@ml-evs
Copy link
Collaborator

ml-evs commented Jan 20, 2024

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?

@chatcannon chatcannon marked this pull request as draft January 20, 2024 20:44
@JhonFlash3008
Copy link
Contributor Author

I prepared a test with a bunch of .mpr and .mpt files from the v11.50. Do you want me to send it ?
Do I juste write it in the test_BioLogic.py file and put the test files in the testdata folder ? or is there a different way to porceed ?

@ml-evs
Copy link
Collaborator

ml-evs commented Jan 22, 2024

I prepared a test with a bunch of .mpr and .mpt files from the v11.50. Do you want me to send it? Do I juste write it in the test_BioLogic.py file and put the test files in the testdata folder ? or is there a different way to porceed ?

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 git lfs install, and then, because they already match the lfs patterns in the .gitattributes file, it should just magically work. Either way it won't break anything if you just commit the files normally, give it a go and we can fix it with follow-up commits!

@JhonFlash3008
Copy link
Contributor Author

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.
I created a new assert_MPR_matches_MPT_v2 method in the test_BioLogic, it uses assert_allclose from numpy.testing as I got messages that assert_field_matches is deprecated. Also it manages EIS quality indicators fields that are different in the mpr and mpt at freq > 100kHz.
I had to modify the method fieldname_to_dtype in the BioLogic to take into account new columns in the mpt, mainly due to PEIS and GEIS files

@JhonFlash3008
Copy link
Contributor Author

JhonFlash3008 commented Jan 22, 2024

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

@chatcannon
Copy link
Collaborator

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 < value > meant and assuming it was the same as value. It should probably be fixed i.e. do not rename the columns in the MPTfile.

chatcannon added a commit that referenced this pull request Feb 6, 2024
Add support for EC-Lab v11.50

Rebased from #95 by @JhonFlash3008
@chatcannon
Copy link
Collaborator

Superseded by #97

@chatcannon chatcannon closed this Feb 6, 2024
JhonFlash3008 pushed a commit to JhonFlash3008/galvani that referenced this pull request Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants