Officially support Python 3.12 and test in CI #3685
Conversation
janosh
added
compatability
|
chgnet/graph/cygraph.c:1208:10: fatal error: fast_converter_libraries/create_graph.c: No such file or directory
1208 | #include "fast_converter_libraries/create_graph.c"
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
error: command '/usr/bin/gcc' failed with exit code 1 |
WalkthroughThis update involves refinements across various modules in the Changes
Possibly related issues
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (3)
Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (6)
tests/io/vasp/test_outputs.py (1)
Line range hint
76-85: Consider removing the print statement unless it is essential for the test, to keep the test output clean.pymatgen/io/vasp/outputs.py (5)
Line range hint
1483-1514: Optimize the parsing of complex coefficients in theWavecarclass.- data = np.fromfile(file, dtype=np.complex64, count=nplane) - np.fromfile(file, dtype=np.float64, count=recl8 - nplane) + data = np.fromfile(file, dtype=np.complex64, count=nplane) + file.seek((recl8 - nplane) * np.dtype(np.float64).itemsize, 1)This change avoids reading unnecessary data into memory by directly seeking to the next relevant position in the file.
Line range hint
1483-1514: Consider using more descriptive variable names in_generate_nbmax.- bmag = np.linalg.norm(self.b, axis=1) - b = self.b + reciprocal_lattice_magnitudes = np.linalg.norm(self.b, axis=1) + reciprocal_lattice_vectors = self.bRenaming
bmagtoreciprocal_lattice_magnitudesandbtoreciprocal_lattice_vectorsenhances readability by making the purpose of these variables clearer.
Line range hint
1483-1514: Refactor the loop in_generate_G_pointsto improve clarity and performance.- for i in range(2 * self._nbmax[2] + 1): - i3 = i - 2 * self._nbmax[2] - 1 if i > self._nbmax[2] else i - for j in range(2 * self._nbmax[1] + 1): - j2 = j - 2 * self._nbmax[1] - 1 if j > self._nbmax[1] else j - for k in range(kmax): - k1 = k - 2 * self._nbmax[0] - 1 if k > self._nbmax[0] else k + for i3 in range(-self._nbmax[2], self._nbmax[2] + 1): + for j2 in range(-self._nbmax[1], self._nbmax[1] + 1): + for k1 in range(-self._nbmax[0], self._nbmax[0] + 1):This change simplifies the loops by directly iterating over the range of indices, removing the need for conditional assignments inside the loop.
Line range hint
1483-1514: Clarify the usage of theshiftparameter in thefft_meshmethod.- return np.fft.ifftshift(mesh) if shift else mesh + shifted_mesh = np.fft.ifftshift(mesh) if shift else mesh + return shifted_meshRefactoring this line to use a temporary variable
shifted_meshimproves readability by making the conditional operation more explicit.
Line range hint
1483-1514: Refine the calculation of band properties ineigenvalue_band_properties.- vbm = -float("inf") - cbm = float("inf") + vbm = np.NINF # Negative infinity using NumPy + cbm = np.inf # Positive infinity using NumPyUsing NumPy's constants for infinity improves readability and ensures compatibility with NumPy operations.
There was a problem hiding this comment.
Actionable comments posted: 4
Out of diff range and nitpick comments (2)
tests/alchemy/test_materials.py (2)
43-45: Consider adding a comment explaining the purpose of theSupercellTransformationin this context.
116-131: The methodto_snlandfrom_snlare used in a way that could be simplified or better documented to explain the transformations and expected outcomes, especially regarding the handling ofother_parametersandhistory.
| alt = ts.append_transformation( | ||
| t_struct = TransformedStructure(struct, []) | ||
| t_struct.append_transformation(SupercellTransformation.from_scaling_factors(2, 1, 1)) | ||
| alt = t_struct.append_transformation( |
There was a problem hiding this comment.
The transformation PartialRemoveSpecieTransformation is applied with a magic number 5. Consider defining this as a constant with a descriptive name to improve readability.
| t_struct = TransformedStructure.from_dict(dct) | ||
| t_struct.other_parameters["author"] = "Will" | ||
| t_struct.append_transformation(SubstitutionTransformation({"Fe": "Mn"})) | ||
| assert t_struct.final_structure.reduced_formula == "MnPO4" | ||
| assert t_struct.other_parameters == {"author": "Will", "tags": ["test"]} |
There was a problem hiding this comment.
The method from_dict modifies other_parameters directly after instantiation. This could lead to side effects if other_parameters is expected to be immutable post-creation. Consider using a method to safely update this property.
| t_struct = TransformedStructure(self.structure, trafos) | ||
| assert t_struct.final_structure.reduced_formula == "NaMnPO4" | ||
| t_struct.undo_last_change() | ||
| assert t_struct.final_structure.reduced_formula == "NaFePO4" | ||
| t_struct.undo_last_change() | ||
| assert t_struct.final_structure.reduced_formula == "LiFePO4" | ||
| with pytest.raises(IndexError, match="No more changes to undo"): | ||
| ts.undo_last_change() | ||
| ts.redo_next_change() | ||
| assert ts.final_structure.reduced_formula == "NaFePO4" | ||
| ts.redo_next_change() | ||
| assert ts.final_structure.reduced_formula == "NaMnPO4" | ||
| t_struct.undo_last_change() | ||
| t_struct.redo_next_change() | ||
| assert t_struct.final_structure.reduced_formula == "NaFePO4" | ||
| t_struct.redo_next_change() | ||
| assert t_struct.final_structure.reduced_formula == "NaMnPO4" |
There was a problem hiding this comment.
The methods undo_last_change and redo_next_change are tested sequentially. It's good practice to separate these into distinct test cases to isolate the effects and make the tests more modular.
pymatgen/io/lobster/inputs.py
Outdated
| def is_float(element) -> bool: | ||
| try: | ||
| float(element) | ||
| return True | ||
| except ValueError: | ||
| return False | ||
|
|
||
| for datum in data: | ||
| # Remove all comments | ||
| if not datum.startswith(("!", "#", "//")): | ||
| pattern = r"\b[^!#//]+" # exclude comments after commands | ||
| if matched_pattern := re.findall(pattern, datum): | ||
| raw_datum = matched_pattern[0].replace("\t", " ") # handle tab in between and end of command | ||
| key_word = raw_datum.strip().split(" ") # extract keyword | ||
| key = key_word[0].lower() | ||
| if len(key_word) > 1: | ||
| # check which type of keyword this is, handle accordingly | ||
| if key_word[0].lower() not in [datum2.lower() for datum2 in Lobsterin.LISTKEYWORDS]: | ||
| if key_word[0].lower() not in [datum2.lower() for datum2 in Lobsterin.FLOAT_KEYWORDS]: | ||
| if key_word[0].lower() not in lobsterin_dict: | ||
| lobsterin_dict[key_word[0].lower()] = " ".join(key_word[1:]) | ||
| if key not in [datum2.lower() for datum2 in Lobsterin.LISTKEYWORDS]: | ||
| if key not in [datum2.lower() for datum2 in Lobsterin.FLOAT_KEYWORDS]: | ||
| if key not in lobsterin_dict: | ||
| lobsterin_dict[key] = " ".join(key_word[1:]) | ||
| else: | ||
| raise ValueError(f"Same keyword {key_word[0].lower()} twice!") | ||
| elif key_word[0].lower() not in lobsterin_dict: | ||
| lobsterin_dict[key_word[0].lower()] = float(key_word[1]) | ||
| raise ValueError(f"Same keyword {key} twice!") | ||
| elif is_float(key_word[1]) and key not in lobsterin_dict: | ||
| lobsterin_dict[key] = float(key_word[1]) | ||
| else: | ||
| raise ValueError(f"Same keyword {key_word[0].lower()} twice!") | ||
| elif key_word[0].lower() not in lobsterin_dict: | ||
| lobsterin_dict[key_word[0].lower()] = [" ".join(key_word[1:])] | ||
| raise ValueError(f"Same keyword {key} twice!") | ||
| elif key not in lobsterin_dict: | ||
| lobsterin_dict[key] = [" ".join(key_word[1:])] | ||
| else: | ||
| lobsterin_dict[key_word[0].lower()].append(" ".join(key_word[1:])) | ||
| lobsterin_dict[key].append(" ".join(key_word[1:])) | ||
| elif len(key_word) > 0: | ||
| lobsterin_dict[key_word[0].lower()] = True | ||
| lobsterin_dict[key] = True |
There was a problem hiding this comment.
The method from_file uses a complex series of conditionals to parse the input file. Consider refactoring this to use a more structured approach or a parsing library to improve maintainability and readability.
|
@JaGeo some of the lobster tests started failing under Python 3.12. i fixed all but two. maybe you know what needs to change here? ____________________ TestLobsterin.test_dict_functionality _____________________
self = <lobster.test_inputs.TestLobsterin testMethod=test_dict_functionality>
def test_dict_functionality(self):
for key in ("COHPstartEnergy", "COHPstartEnergy", "COhPstartenergy"):
start_energy = self.Lobsterin.get(key)
> assert start_energy == -15.0, f"{start_energy=}"
E AssertionError: start_energy=None
tests/io/lobster/test_inputs.py:1773: AssertionError
___________________ TestLobsterin.test_read_write_lobsterin ____________________
self = <lobster.test_inputs.TestLobsterin testMethod=test_read_write_lobsterin>
def test_read_write_lobsterin(self):
outfile_path = tempfile.mkstemp()[1]
lobsterin1 = Lobsterin.from_file(f"{TEST_DIR}/lobsterin.1")
lobsterin1.write_lobsterin(outfile_path)
lobsterin2 = Lobsterin.from_file(outfile_path)
> assert lobsterin1.diff(lobsterin2)["Different"] == {}
E AssertionError
tests/io/lobster/test_inputs.py:1793: AssertionError |
|
@janosh could you try this solution for now? |
…nd modify __getitem__ to get the salient __getitem__ behavior from UserDict
|
@JaGeo oh right, i forgot about our if possible, i'd prefer to drop |
|
@JaGeo the suggested fix you linked doesn't work (see da1f7c5). i'm still unclear what benefits |
|
I can try again in the next days. Need to fix other stuff as well. Sorry that this functionality here is such a pain. |
|
the thing making this hard to debug is that i can't repro the issue locally. even on 3.12, everything works for me on macOS. don't understand why this would be linux-specific (if it is) |
Fixes everything except for a temp_path problem. Not sure where this is coming from. will check and see if I can fix it as well. Overwrote the |
|
@janosh tests are passing locally now for py 3.12. You might want to check the tmp_dir change. The rest should hopefully be fine now. |
|
3.9 and 3.12 tests are passing. 3.11 does not run. |
perfect! thanks! 👍
that's intentional. no point in wasting Ci budget. usually enough to test the oldest and newest boundaries of supported versions. i'll update the required status checks before merging here to make 3.12 tests instead of 3.11 required
done. i refactored the test slightly and deleted the new test files. i assume they were added by accident? |
janosh
changed the title
unknown if all upstream packages have 3.12 support by now. potential issues expected for
pytorchandspglibmaterialsproject/atomate2#768 (comment).