bug fix for potcar parsing #2910
Conversation
Agreed. |
|
So this is still failing, I really hope this is somewhat connected to the massive amount of warnings I always get during POTCAR reading so w/e fixes this will fix that as well. Maybe we have to redo the hashes and be more consistent about trimming? Again I'm not too familiar with that part of the code. |
|
I have merged this first. But I really really dislike the whole hash system. Unless someone gives me a good reason why we are spending so much effort on this, I am disabling the tests and in future, I will disable the hash checks for good as well. |
I don't see a reason. Just to be clear, you mean keep all the Potcar tests and just disable those that relate to the hashes? I think that would be good. |
|
@shyuep I think 99% of people only rely on the headers. I recall there were "secret" custom PAWs with the same headers as standard ones that people used (which later got released under a different header). But I feel like if you are using those then it's on the user to sort that out. However, I assume the hashing was added to get to make sure the build process is a bit more careful (and might be used by the builders) so you might need to ping @munrojm before any major changes. |
|
I agree. So why don't you comment out the offending tests in the PR to let it pass. We will then merge. I think the hash checks can throw warnings like they do currently, but I am really tired of these tests failing because of some minor missing new line or space somewhere. |
|
As long as the hashing functionality remains in the code, we should be okay. We currently use hash checking as part of the calculation validation pipeline. |
| @@ -2275,19 +2275,19 @@ def from_file(filename: str): | |||
|
|
|||
| functionals = [] | |||
| for p in fdata.split("End of Dataset"): | |||
| if p.strip(): | |||
| single = PotcarSingle(p + "End of Dataset\n") | |||
| if p_strip := p.strip(): | |||
There was a problem hiding this comment.
Ooo walrus is in 3.8? nice!
|
@munrojm I will leave the hash generation in. But if we are disabling the tests, I make no guarantees that future hashes generated will be valid. In other words, the hash is just being generated and we do not test for correctness. As I said, this is far too much effort to maintain for a functionality that very few people care about. |
|
@shyuep, that is fair. |
|
Can this get merged, please? I think the failing tests are from an unrelated issue. |
Head branch was pushed to by a user without write access
I think this is waiting on commenting out the failing POTCAR hash tests as @shyuep suggested. |
|
@janosh I modified to test and now it just attempts parsing withouth any checks. I actually wasn't sure if it was proper etiquette for someon other than the maintainers to delete tests which is why I didn't do it originally. |
bug fix for potcar parsing
2023.3.10 rewrote the
PotcarSingle.from_filefunction slightly.This caused a parsing error in the tests for
pymatgen-analysis-defects.I think this small change gives the intended behavior.
Otherwise, the POTCAR in this folder cannot be parsed.
https://github.com/materialsproject/pymatgen-analysis-defects/tree/main/tests/test_files/v_Ga/ccd_0_-1/optics
Without the fix:
Gives:
with the fix, you get: