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

Fix CI failure: bump netcdf4, replace Namespace with standard dict for io.abinit.pseudos #4223

Merged

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Dec 11, 2024

Fix CI failure, to fix #4221

TODOs:

  • investigate netcdf4 compatibility with NP2
  • double check latest monty compatibility

@DanielYang59 DanielYang59 force-pushed the fix-ci-failure-2024-12-11 branch from 619eb2e to 2b37fc9 Compare December 11, 2024 07:27
@@ -601,7 +601,9 @@ def _dict_from_lines(lines, key_nums, sep=None) -> dict:
if len(lines) != len(key_nums):
raise ValueError(f"{lines = }\n{key_nums = }")

kwargs = Namespace()
Copy link
Contributor Author

@DanielYang59 DanielYang59 Dec 11, 2024

Choose a reason for hiding this comment

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

It turns out the issue is two-fold:
My fault goes first, looks like the override on update is not complete to handle Iterator correctly, and surely I would fix this ASAP:

from monty.collections import Namespace

namespace_dict = Namespace()

namespace_dict.update(zip(["a", "b"], [1, 2]))
print(namespace_dict)  # {}

namespace_dict.update({"a":1, "b":2})
print(namespace_dict)  # {'a': 1, 'b': 2}

After fixing this issue, the usage of Namespace in oncvpsp_header method of io.abinit.pseudo.NcAbinitHeader seems unexpected to me around pop:

header.pop("pspd")

According to the original docstring (before my clean up in materialsvirtuallab/monty#722):

A dictionary that does not permit to redefine its keys.

As such I would consider pop "redefined its keys" as it removed the key from the dictionary.

Currently I don't see any error with replacing Namespace dict by a standard dict, so I might use standard dict for now until someone have any comment.

@DanielYang59 DanielYang59 marked this pull request as ready for review December 11, 2024 10:37
@DanielYang59 DanielYang59 changed the title Fix CI failure Fix CI failure: bump netcdf4, replace Namespace with standard dict for io.abinit.pseudos Dec 11, 2024
@DanielYang59
Copy link
Contributor Author

@shyuep This should fix failure.

Regarding the Namespace issue from monty I would fix it ASAP but I don't think it's relevant anymore, i.e. after we fixed that implementation it still cannot be used (intentionally) as pop would not be allowed which "redefines the keys".

@shyuep shyuep merged commit 362bf54 into materialsproject:master Dec 11, 2024
43 checks passed
@DanielYang59 DanielYang59 deleted the fix-ci-failure-2024-12-11 branch December 11, 2024 14:38
@DanielYang59
Copy link
Contributor Author

Thanks!

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