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

Extend param_map #191

Closed
wants to merge 4 commits into from
Closed

Conversation

carla-terboven
Copy link
Contributor

Belongs to issue #190

This PR extends the parammap for _vs params. The mapping is certainly not complete but is based on all the files currently available to me.

@PeterKraus
Copy link
Contributor

PeterKraus commented Oct 16, 2024

Thank you! The CI fails because of a timezone bug that has been fixed in main. That can be fixed by rebasing from upstream and pushing this branch:

git pull --rebase upstream main
git push -f origin main

Also, something is not right with the formatting (I'm guessing trailing spaces); that can be fixed by running:

ruff format .

on the codebase.

The new data will have to be hooked up into the actual extractor and include some tests. In particular, we can use our existing mpr and mpt file pairs to check for more known int->str conversions.

For the mpr files, this is done here:

pardict[k] = param_from_key(k, v, to_str=True)

using this function:

def param_from_key(

Please let me know if you're keen to contribute this or if I should do it.

@carla-terboven
Copy link
Contributor Author

I have now compared all files from the tests/test_x_eclab_mpr/ folder with the comparable files from tests/test_x_eclab_mpt/.
I only left out the mb files with the _vs params ctrl1_val_vs, ctrl2_val_vs, ctrl3_val_vs.
I pushed the result here 5a19ed0.

Problem: a few mappings were already assigned.
So the test files provide the following additional mappings:

"E_vs": (("Eoc", 0),),
"Ef_vs": (("Ei", 3),),
"Ei_vs": (("Emeas", 2),),

although I had already found these before:

"E_vs": (("Eref", 0),),
"Ef_vs": (("Eoc", 3),),
"Ei_vs": (("Eref", 2),),

Unfortunately, I don't know how to deal with these differences.

@PeterKraus
Copy link
Contributor

That's tricky. It might well be that different techniques have a different parameter map. It might also be that the parameters are not parsed properly in one of these conflicting files. I might have a look at it over the weekend.

@PeterKraus
Copy link
Contributor

So I had a quick look and I found a couple of bugs in parameter parsing for some of the techniques. In particular, parameters for MP, coV and CV (for 11.50) were not parsed correctly, leading to indices higher than 255.

This fix will be in #193; I will need to regenerate most of the test cases. I suggest we can fix the mapping in this PR once the parsing is done properly, if that's OK with you?

@PeterKraus
Copy link
Contributor

@carla-terboven I have now merged #193, so that the parameters between mpr and mpt files are passed consistently. Could you please check any files you might have with the current main branch, see if there is anything failing, and incorporate what's missing? It may well be that most of the values you had in your parameter map have been added already, or were bugs.

Finally, feel free to add yourself to the contributors list in the README.md file. Thanks a lot for your help!

@carla-terboven
Copy link
Contributor Author

Thank you for your help @PeterKraus!
#193 works for almost all my files. I have opened #196 for everything else. Therefore, I will close this PR now.

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