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

Add support for EC-Lab v11.50 (rebased) #97

Merged
merged 5 commits into from
Feb 6, 2024

Conversation

chatcannon
Copy link
Collaborator

@chatcannon chatcannon commented Jan 20, 2024

A rebased version of #95 with the CRLF changes undone and the black formatting split to a separate commit.

@JhonFlash3008 can you check to see if this is what you wanted?

Closes #87.

Few modifications in the VMPdata_dtype_from_colIDs
Added new headers VMPmodule_hdr_v2
Modified MPRfile initialization

Includes squashed linting fixes by @ml-evs
Copy link
Contributor

@JhonFlash3008 JhonFlash3008 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the changes are ok

galvani/BioLogic.py Outdated Show resolved Hide resolved
galvani/BioLogic.py Outdated Show resolved Hide resolved
galvani/BioLogic.py Outdated Show resolved Hide resolved
@JhonFlash3008
Copy link
Contributor

Sorry for the mess with the formatting. In the future I'll make sure to submit in unix formatting. About the Black autoformat, if you don't want I can get rid of it when I work on this project

@chatcannon chatcannon marked this pull request as ready for review February 3, 2024 12:26
@chatcannon
Copy link
Collaborator Author

@ml-evs I included the latest changes from #95 so if you're happy with this then let's merge it.

@ml-evs
Copy link
Collaborator

ml-evs commented Feb 3, 2024

@chatcannon could we hold off a day or two more? I've been sent a load of example files now that the ECLab "upgrade" is rolling out in our lab, some of them don't contain the parsed data I would expect and as of right now I haven't been able to debug why yet. Hoping to grab some time later today or tomorrow to try again with the latest state of this PR.

Copy link
Collaborator

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, had time to investigate and it was a "me problem", all the tests files I was provided now work well with this PR! No real comments on the code itself; if breaking changes to the MPR format becomes a common occurrence then we might have to restructure along the more automated lines discussed in the issue, but this is good for now.

The only change I had to make downstream to get essentially the same response back from this version of galvani and the last is that the column name for dQ/mA.h changes to dq/mA.h, even for older MPR files. Perhaps @chatcannon could just verify if this is necessary before merging (I've left a comment at the point where this changed in the code).

Either way, thanks @JhonFlash3008 for making the time to contribute this back, and @chatcannon for guidance!

galvani/BioLogic.py Show resolved Hide resolved
@chatcannon chatcannon merged commit 1fd9f84 into echemdata:master Feb 6, 2024
4 checks passed
@chatcannon chatcannon deleted the JhonFlash-master branch February 6, 2024 19:43
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.

Incompatibility with EC-Lab>=11.50 - BioLogic.MPRfile issue
3 participants