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

Refine params dtype #196

Merged
merged 7 commits into from
Nov 1, 2024
Merged

Conversation

carla-terboven
Copy link
Contributor

@carla-terboven carla-terboven commented Oct 22, 2024

First of all: Thank you very much for all the changes in PR #193. That was a wonderful surprise after the weekend.

As described in issue #195 I had a few minor problems for a few of my LSV, GEIS and PEIS files. These are fixed in this PR.

Currently I am trying to get non-confidential files as test files. I hope to be able to add these to the PR in the next few days. Or do you happen to have any matching files?

My adjustments are mainly the result of trial and error. Maybe you can have a look if the changes seem to make sense @PeterKraus.

@carla-terboven carla-terboven mentioned this pull request Oct 22, 2024
Copy link
Contributor

@PeterKraus PeterKraus left a comment

Choose a reason for hiding this comment

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

This looks great, thanks a lot for your contribution!

There are a few parameters that are now inconsistent between the 11.50 and 10.40 versions - it would be wonderful if you could check whether the 11.50 values would break tests for the 10.40 files in the test suite.

Also, the LSV version 11.50 should really have a test file, ideally with multiple Ns. The rest we can live without.

src/yadg/extractors/eclab/techniques.py Show resolved Hide resolved
src/yadg/extractors/eclab/techniques.py Show resolved Hide resolved
src/yadg/extractors/eclab/techniques.py Show resolved Hide resolved
src/yadg/extractors/eclab/techniques.py Show resolved Hide resolved
211: ("<f8", "Q charge or discharge", "C"),
217: ("<f4", "THD Ewe", "%"),
241: ("<f4", "|E1|", "V"),
242: ("<f4", "|E2|", "V"),
271: ("<f4", "Phase(Z1)", "deg"),
272: ("<f4", "Phase(Z2)", "deg"),
# 272: ("<f4", "Phase(Z2)", "deg"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to change Phase(Z2) to Analog IN 1 for my testfile to pass. I could not find any Phase(Z2) in the mpt test files so I am not sure how to debug.

Copy link
Contributor

Choose a reason for hiding this comment

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

Strange. I think Analog IN 1 must be correct. Can you please delete the commented line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line is now deleted in 610a36c

Copy link
Contributor

@PeterKraus PeterKraus left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the test files! Once you remove that one commented line, it's good to go!

@PeterKraus PeterKraus merged commit 0b1f8d4 into dgbowl:main Nov 1, 2024
12 checks passed
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.

2 participants