Remove deprecated analysis.interface, rename classes to PascalCase and rename with_* to from_*
#3725
Conversation
WalkthroughThe recent updates to the pymatgen library focus on standardizing method naming conventions, particularly in the transition from methods prefixed with Changes
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 (
|
|
bear in mind that we need deprecations for all these changes. might be easier to do them alongside each change rather than all at once at the end to ensure we don't forget any |
|
It looks like my method to deprecate class methods ( def wrapped(*args, **kwargs):
msg = craft_message(old, replacement, message)
warnings.warn(msg, category=category, stacklevel=2)
> return old(*args, **kwargs)
E TypeError: 'classmethod' object is not callable |
Agreed :) Already moved |
|
Please review @janosh. Thanks! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (17)
- pymatgen/analysis/bond_dissociation.py (2 hunks)
- pymatgen/analysis/dimensionality.py (1 hunks)
- pymatgen/analysis/fragmenter.py (2 hunks)
- pymatgen/analysis/functional_groups.py (1 hunks)
- pymatgen/analysis/graphs.py (17 hunks)
- pymatgen/analysis/local_env.py (8 hunks)
- pymatgen/analysis/magnetism/heisenberg.py (2 hunks)
- pymatgen/analysis/quasiharmonic.py (2 hunks)
- pymatgen/command_line/critic2_caller.py (1 hunks)
- pymatgen/io/cp2k/inputs.py (8 hunks)
- pymatgen/io/cp2k/sets.py (11 hunks)
- pymatgen/io/qchem/outputs.py (1 hunks)
- tests/analysis/test_fragmenter.py (3 hunks)
- tests/analysis/test_functional_groups.py (1 hunks)
- tests/analysis/test_graphs.py (27 hunks)
- tests/analysis/test_local_env.py (5 hunks)
- tests/io/test_babel.py (1 hunks)
Additional Context Used
Additional comments not posted (79)
tests/analysis/test_functional_groups.py (1)
32-32: Renamingwith_local_env_strategytofrom_local_env_strategyaligns with the PR's objective of improving method naming clarity. Ensure that this change does not alter the intended functionality of the method or affect the outcome of the tests.tests/io/test_babel.py (1)
56-56: RenamingMoleculeGraph.with_empty_graphtoMoleculeGraph.from_empty_graphaligns with the PR's objective of improving method naming clarity. Ensure that this change does not alter the intended functionality of the method or affect the outcome of the tests.tests/analysis/test_fragmenter.py (3)
75-75: RenamingMoleculeGraph.with_local_env_strategytoMoleculeGraph.from_local_env_strategyaligns with the PR's objective of improving method naming clarity. Ensure that this change does not alter the intended functionality of the method or affect the outcome of the tests.
84-84: RenamingMoleculeGraph.with_local_env_strategytoMoleculeGraph.from_local_env_strategyaligns with the PR's objective of improving method naming clarity. Ensure that this change does not alter the intended functionality of the method or affect the outcome of the tests.
99-99: RenamingMoleculeGraph.with_edgestoMoleculeGraph.from_edgesaligns with the PR's objective of improving method naming clarity. Ensure that this change does not alter the intended functionality of the method or affect the outcome of the tests.pymatgen/analysis/functional_groups.py (1)
89-89: Renamingwith_local_env_strategytofrom_local_env_strategyaligns with the PR's objective of improving method naming clarity. Ensure that this change does not alter the intended functionality of the method or affect the functionality of theFunctionalGroupExtractorclass.pymatgen/analysis/quasiharmonic.py (2)
229-229: The change in the class name fromQuasiharmonicDebyeApproxtoQuasiHarmonicDebyeApproxaligns with the PR's objective of ensuring consistency in naming conventions. Ensure that this change does not alter the intended functionality of the class or affect its usage across the codebase.
364-364: The modification in the deprecation message to remove the recommendation to use PascalCase is appropriate, as the class name already adheres to PascalCase. Ensure that the deprecation process is communicated clearly to users.pymatgen/analysis/fragmenter.py (3)
83-83: The methodMoleculeGraph.from_local_env_strategyis correctly used to initializeMoleculeGraphbased on the local environment strategy. This change aligns with the PR's objective to rename methods for clarity.
86-86: The methodMoleculeGraph.from_edgesis correctly used to initializeMoleculeGraphwith specified edges. This change is consistent with the renaming objective and improves clarity in method naming.
310-310: Theopen_ringfunction now correctly returns aMoleculeGraphusing thefrom_local_env_strategymethod. This change is in line with the PR's objective to rename methods for improved clarity.pymatgen/analysis/bond_dissociation.py (3)
93-93: The methodMoleculeGraph.from_local_env_strategyis correctly used to initializeMoleculeGraphfor the principle molecule graph. This change aligns with the PR's objective to rename methods for clarity.
290-290: The methodMoleculeGraph.from_local_env_strategyis correctly used to initialize theinitial_molgraphfrom the initial molecule. This change is consistent with the renaming objective and improves clarity in method naming.
293-293: The methodMoleculeGraph.from_local_env_strategyis correctly used to initialize thefinal_molgraphfrom the final molecule. This change is in line with the PR's objective to rename methods for improved clarity.pymatgen/analysis/dimensionality.py (1)
290-290: The methodMoleculeGraph.from_edgesis correctly used in thezero_d_graph_to_molecule_graphfunction to construct aMoleculeGraphfrom edges. This change aligns with the PR's objective to rename methods for clarity and is appropriate for initializingMoleculeGraphinstances based on edges.pymatgen/command_line/critic2_caller.py (1)
532-532: The change fromStructureGraph.with_empty_graphtoStructureGraph.from_empty_graphaligns with the PR's objective to rename methods for improved clarity. This change is correctly applied and enhances the readability and intuitiveness of the method call.pymatgen/analysis/magnetism/heisenberg.py (2)
119-119: The methodStructureGraph.from_local_env_strategyhas been correctly renamed toStructureGraph.from_local_env_strategyas part of the PR's objective to improve method naming clarity. This change aligns with the Python community's convention of usingfrom_*for class methods that instantiate objects in a specific way.
545-545: The methodStructureGraph.from_empty_graphhas been correctly renamed toStructureGraph.from_empty_graph, following the PR's objective to rename methods for improved clarity. This change is consistent with the convention of usingfrom_*for class methods that instantiate objects in a specific way.tests/analysis/test_graphs.py (25)
56-56: The method callStructureGraph.from_empty_graphcorrectly reflects the PR's objective to rename methods for clarity. This change fromwith_empty_graphtofrom_empty_graphimproves the semantic understanding of the method's purpose, aligning with common Python practices for class method naming that create instances from specific states or data.
109-109: The method callStructureGraph.from_local_env_strategycorrectly replaces the previouswith_local_env_strategy, adhering to the PR's objective of renaming methods for improved clarity. This change enhances the readability and intuitive understanding of the method's functionality, which is to create aStructureGraphinstance based on a local environment strategy.
137-137: The usage ofStructureGraph.from_local_env_strategyhere is consistent with the PR's goal of renaming methods for clarity. This change makes it clearer that the method is constructing a newStructureGraphinstance from a local environment strategy, which is a more intuitive description of the method's functionality.
209-209: The method callStructureGraph.from_local_env_strategyis correctly updated fromwith_local_env_strategy, aligning with the PR's objectives. This renaming enhances the method's semantic clarity, indicating that it creates aStructureGraphinstance based on a local environment strategy.
228-228: The use ofStructureGraph.from_local_env_strategyhere follows the PR's intention of renaming methods for improved clarity. This change is beneficial as it more accurately describes the method's purpose of constructing aStructureGraphinstance from a local environment strategy, improving code readability.
234-234: The method callStructureGraph.from_empty_graphis correctly updated to reflect the PR's objective of renaming methods for clarity. This change fromwith_empty_graphtofrom_empty_graphimproves the semantic understanding of the method's purpose, aligning with common Python practices for class method naming that create instances from specific states or data.
333-333: The method callStructureGraph.from_local_env_strategyis correctly updated fromwith_local_env_strategy, aligning with the PR's objectives. This renaming enhances the method's semantic clarity, indicating that it creates aStructureGraphinstance based on a local environment strategy.
338-338: The usage ofStructureGraph.from_local_env_strategyhere is consistent with the PR's goal of renaming methods for improved clarity. This change makes it clearer that the method is constructing a newStructureGraphinstance from a local environment strategy, which is a more intuitive description of the method's functionality.
359-359: The method callStructureGraph.from_local_env_strategyis correctly updated fromwith_local_env_strategy, aligning with the PR's objectives. This renaming enhances the method's semantic clarity, indicating that it creates aStructureGraphinstance based on a local environment strategy.
401-401: The use ofStructureGraph.from_local_env_strategyhere follows the PR's intention of renaming methods for improved clarity. This change is beneficial as it more accurately describes the method's purpose of constructing aStructureGraphinstance from a local environment strategy, improving code readability.
406-406: The method callStructureGraph.from_local_env_strategycorrectly reflects the PR's objective to rename methods for clarity. This change fromwith_local_env_strategytofrom_local_env_strategyimproves the semantic understanding of the method's purpose, aligning with common Python practices for class method naming that create instances from specific states or data.
427-427: The usage ofStructureGraph.from_edgeshere is consistent with the PR's goal of renaming methods for improved clarity. This change makes it clearer that the method is constructing a newStructureGraphinstance from a set of edges, which is a more intuitive description of the method's functionality.
437-437: The method callStructureGraph.from_local_env_strategyis correctly updated fromwith_local_env_strategy, aligning with the PR's objectives. This renaming enhances the method's semantic clarity, indicating that it creates aStructureGraphinstance based on a local environment strategy.
475-475: The use ofStructureGraph.from_local_env_strategyhere follows the PR's intention of renaming methods for improved clarity. This change is beneficial as it more accurately describes the method's purpose of constructing aStructureGraphinstance from a local environment strategy, improving code readability.
573-573: The method callMoleculeGraph.from_edgescorrectly reflects the PR's objective to rename methods for clarity. This change fromwith_edgestofrom_edgesimproves the semantic understanding of the method's purpose, aligning with common Python practices for class method naming that create instances from specific states or data.
584-584: The usage ofMoleculeGraph.from_edgeshere is consistent with the PR's goal of renaming methods for improved clarity. This change makes it clearer that the method is constructing a newMoleculeGraphinstance from a set of edges, which is a more intuitive description of the method's functionality.
594-594: The method callMoleculeGraph.from_edgesis correctly updated to reflect the PR's objective of renaming methods for clarity. This change fromwith_edgestofrom_edgesimproves the semantic understanding of the method's purpose, aligning with common Python practices for class method naming that create instances from specific states or data.
595-595: The method callMoleculeGraph.from_local_env_strategycorrectly replaces the previouswith_local_env_strategy, adhering to the PR's objective of renaming methods for improved clarity. This change enhances the readability and intuitive understanding of the method's functionality, which is to create aMoleculeGraphinstance based on a local environment strategy.
605-605: The usage ofMoleculeGraph.from_local_env_strategyhere, despite being within a test for an inappropriate strategy, is consistent with the PR's goal of renaming methods for improved clarity. The change makes it clearer that the method is intended to construct a newMoleculeGraphinstance from a local environment strategy, even though this particular case tests for an error condition.
700-700: The method callMoleculeGraph.from_empty_graphis correctly updated to reflect the PR's objective of renaming methods for clarity. This change fromwith_empty_graphtofrom_empty_graphimproves the semantic understanding of the method's purpose, aligning with common Python practices for class method naming that create instances from specific states or data.
714-714: The usage ofMoleculeGraph.from_empty_graphhere is consistent with the PR's goal of renaming methods for improved clarity. This change makes it clearer that the method is constructing a newMoleculeGraphinstance from an empty graph, which is a more intuitive description of the method's functionality.
788-788: The method callMoleculeGraph.from_edgescorrectly reflects the PR's objective to rename methods for clarity. This change fromwith_edgestofrom_edgesimproves the semantic understanding of the method's purpose, aligning with common Python practices for class method naming that create instances from specific states or data.
835-835: The usage ofMoleculeGraph.from_edgeshere is consistent with the PR's goal of renaming methods for improved clarity. This change makes it clearer that the method is constructing a newMoleculeGraphinstance from a set of edges, which is a more intuitive description of the method's functionality.
843-843: The method callMoleculeGraph.from_edgesis correctly updated to reflect the PR's objective of renaming methods for clarity. This change fromwith_edgestofrom_edgesimproves the semantic understanding of the method's purpose, aligning with common Python practices for class method naming that create instances from specific states or data.
847-847: The usage ofMoleculeGraph.from_edgeshere is consistent with the PR's goal of renaming methods for improved clarity. This change makes it clearer that the method is constructing a newMoleculeGraphinstance from a set of edges, which is a more intuitive description of the method's functionality.tests/analysis/test_local_env.py (6)
14-16: Renaming of classesBrunnerNN_real,BrunnerNN_reciprocal, andBrunnerNN_relativetoBrunnerNNReal,BrunnerNNReciprocal, andBrunnerNNRelativealigns with Python's naming conventions and improves readability.
418-430: The instantiation ofBrunnerNNReciprocal,BrunnerNNRelative, andBrunnerNNRealclasses with the updated names is correctly done. The method calls and assertions within these tests are logically sound and adhere to the testing framework's guidelines.
1339-1345: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1342-1372]
The creation of
MoleculeGraphforphshusingfrom_edgesmethod is correctly implemented. The edges dictionary accurately represents the molecular structure, which is crucial for the subsequent tests involvingoxygen_edge_extender.
1391-1405: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1372-1402]
Similarly, the creation of
MoleculeGraphforLiECis correctly done. The edges dictionary is well-defined, setting up a proper test environment formetal_edge_extender.
1394-1402: The instantiation ofMoleculeGraphfor the water cluster around potassium (water_cluster_K) with an empty graph is a good setup for testing themetal_edge_extenderfunction. It's important to ensure that the test accurately reflects the intended use case of the function.
418-430: Using the updated class names in the test cases ensures consistency with the main codebase changes. It's good practice to keep test cases updated to reflect changes in the code they are testing.pymatgen/io/cp2k/sets.py (10)
317-323: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [93-320]
The constructor of
DftSetis well-implemented with comprehensive documentation. However, given its complexity, consider refactoring to improve readability and maintainability. For instance, breaking down the setup process into smaller, more focused methods could enhance clarity and make the code easier to follow.
666-677: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [320-674]
The
get_basis_and_potentialmethod is comprehensive and handles various strategies for specifying basis and potential information. To improve readability, consider refactoring by extracting some of the logic into smaller, helper functions. This could make the method easier to follow and maintain.
666-677: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [674-688]
The
get_cutoff_from_basismethod is efficiently implemented and clearly documented. Good use of numpy operations for handling exponents.
834-840: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [688-927]
The
get_xc_functionalsmethod is concise and effectively handles the retrieval and expansion of XC functional names. Well done.
834-840: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [837-860]
The
write_potential_filemethod is effectively implemented, using thePotentialFileclass to write potentials to a file. Consistent with the approach used for writing basis sets.
857-863: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [860-886]
The
print_forcesmethod correctly configures the CP2K input to print out forces and stress during the calculation. Well-implemented.
883-889: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [886-927]
The
print_dosmethod is well-documented and includes necessary conditional checks for compatibility with k-point calculations. The considerations for settingndigitsare clearly explained.
927-927: The remaining methods inDftSet, such asprint_pdos,print_ldos, etc., are consistently implemented and well-documented. The approach to configuring print settings for the CP2K input is clear and effective.
927-927: TheStaticSetclass provides a convenient way to set up static calculations by correctly inheriting fromDftSetand specifying parameters for static calculations.
927-927: The remaining classes, such asRelaxSet,CellOptSet, etc., are consistently implemented and serve as convenient constructors for specific types of calculations. The inheritance fromDftSetis correctly utilized across these classes.pymatgen/io/cp2k/inputs.py (10)
1589-1589: The changes toVHartreeCubelook good. Ensure its integration with the rest of the system is verified.
1609-1611: The deprecation ofV_Hartree_Cubein favor ofVHartreeCubeis handled correctly. Ensure all references toV_Hartree_Cubethroughout the codebase are updated toVHartreeCube.
1614-1614: The changes toMOCubeslook good. Ensure its integration with the rest of the system is verified.
1654-1654: The deprecation ofMO_Cubesin favor ofMOCubesis handled correctly. Ensure all references toMO_Cubesthroughout the codebase are updated toMOCubes.
1658-1658: The changes toEDensityCubelook good. Ensure its integration with the rest of the system is verified.
1679-1679: The deprecation ofE_Density_Cubein favor ofEDensityCubeis handled correctly. Ensure all references toE_Density_Cubethroughout the codebase are updated toEDensityCube.
1849-1849: The changes toXCFunctionallook good. Ensure its integration with the rest of the system is verified.
1878-1878: The deprecation ofXc_Functionalin favor ofXCFunctionalis handled correctly. Ensure all references toXc_Functionalthroughout the codebase are updated toXCFunctional.
2106-2106: The changes toBandStructurelook good. Ensure its integration with the rest of the system is verified.
2186-2186: The deprecation ofBand_Structurein favor ofBandStructureis handled correctly. Ensure all references toBand_Structurethroughout the codebase are updated toBandStructure.pymatgen/analysis/graphs.py (3)
19-19: The import ofdeprecatedfrommonty.devis added to support the deprecation warnings for the renamed methods. Ensure that themontypackage is included in the project dependencies.
198-212: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [157-207]
Renaming methods from
with_tofrom_and adding deprecation warnings is a good practice for maintaining backward compatibility while signaling the future removal of these methods. However, ensure that all references to these methods throughout the codebase and documentation are updated to reflect the new names.Verification successful
The shell script outputs indicate that the methods
with_empty_graph,with_edges, andwith_local_env_strategyare still present in thepymatgen/analysis/graphs.pyfile. This presence is consistent with the practice of deprecating methods while introducing new ones with updated names for backward compatibility. Therefore, the review comment is verified based on the information available, but it's important to ensure that these deprecated methods are properly marked and documented as such, and that new references in the codebase use the updated method names.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to the old method names to ensure they've been updated. rg --type py "with_empty_graph" rg --type py "with_edges" rg --type py "with_local_env_strategy"Length of output: 554
202-204: The deprecation message format is consistent across the renamed methods, providing clear information on the deprecation date and the removal date. This consistency helps in maintaining a standard approach to deprecation across the codebase.pymatgen/io/qchem/outputs.py (1)
2231-2233: The implementation ofcheck_for_structure_changesusingMoleculeGraphwithOpenBabelNNis a robust approach for comparing molecular connectivity. However, consider the following points for refinement:
- The handling of
special_elementsand their distances might not be fully utilized in the current logic. If there's a specific reason for identifying these elements and their distances, ensure that this information is effectively used or clarify its purpose.- The method returns strings to indicate the type of structural change. While this works, it might be beneficial to define these return values as constants or an Enum to avoid potential issues with typos and to facilitate easier handling of these states elsewhere in the code.
- Ensure that the method's documentation clearly explains the return values and under what conditions each is returned.
pymatgen/analysis/local_env.py (6)
18-24: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [93-93]
The
ValenceIonicRadiusEvaluatorclass is well-implemented and follows best practices for encapsulation and method responsibilities.
18-24: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1025-1025]
The
NearNeighborsbase class provides a clear and concise interface for subclasses to implement specific near neighbor finding algorithms.
3604-3604: TheCritic2NNclass introduces a dependency on critic2 for topological analysis. It's important to ensure that the critic2 dependency is properly managed and documented, including installation instructions for users who may not have critic2 installed.
3449-3449: TheCutOffDictNNclass provides a simple and effective way to define near neighbor relationships using a dictionary of fixed cut-off distances. It would be helpful to include examples or guidance on configuring the cut-off distances for common use cases to assist users in effectively utilizing this class.
3604-3604: TheCrystalNNclass offers a flexible and comprehensive approach to determining near neighbors in periodic structures. Given the complexity and the number of parameters available for tuning the analysis, enhancing the documentation to cover the use of these parameters and their impact on the neighbor finding process would greatly benefit users.
3604-3604: Theoxygen_edge_extenderandmetal_edge_extenderfunctions provide valuable utility in identifying and adding missed bonds in molecular structures. Reviewing the distance criteria and element types used in these functions to ensure they are suitable for a wide range of molecular structures would be beneficial.
|
Thanks again for reviewing @janosh! Cheers! |
DanielYang59
changed the title
analysis.interface, rename classes to PascalCase and rename with_* to from_*analysis.interface, rename classes to PascalCase and rename with_* to from_*
janosh
added
housekeeping
Summary
analysis.interface(deprecated 3 years ago, see discussion)with_*classmethods tofrom_*inanalysis.graphsNote:
The renamed classes in
cp2k.inputsdoes not have corresponding unit tests at all.Summary by CodeRabbit