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

Replace general raise Exception and add missing raise keyword #3728

Merged
merged 9 commits into from
Mar 30, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Mar 30, 2024

Summary

Major changes:

  • Some Exception is missing the raise keyword (thought there would be some as I did notice some in previous clean up, but luckily just found one in fae74b0) (tried re pattern: ^\s?(?!raise)\s+\w+error\(\w?, ^\s?(?!raise)\s+\w?exception\(\w?)
  • Replace too general raise Exception with specific Error

Reverted for now:

  • Suppress Exception with contextlib.suppress for simplicity (use re pattern: except\s+.*:\n\s+pass)

Copy link

coderabbitai bot commented Mar 30, 2024

Walkthrough

The recent updates across various components of a widely used library focus primarily on refining error handling mechanisms. By replacing generic exceptions with more specific ones like RuntimeError, ValueError, and KeyError, as well as ensuring that NotImplementedError is properly raised, the changes aim to enhance the clarity and precision of error messages, thereby improving the debugging experience for developers.

Changes

File Path Change Summary
.../coordination_geometries.py, .../operations.py, .../bandstructure.py, .../advanced_transformations.py Replaced Exception with RuntimeError for improved error messaging.
.../battery/battery_abc.py Changed NotImplementedError to raise NotImplementedError for consistency.
.../adf.py Changed raised exception from Exception to KeyError for specificity.
.../io/cif.py Improved exception handling by replacing generic exceptions with ValueError and NotImplementedError.
.../io/vasp/outputs.py Enhanced error handling by using specific exceptions like ValueError and RuntimeError.

🐰✨
Changes abound, errors more clear,
A rabbit hops, spreading cheer.
No more confusion in the night,
With precise errors, code takes flight.
Let's raise a carrot, toast the day,
For clearer paths, we've found our way.
🥕🌟

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

thanks @DanielYang59. it's always nice to reduce code lines but contextlib.suppress is slower than try/except. not sure if it's enough to be noticeable but i think remember reading somewhere it can be quite a lot especially if used in a for loop. perhaps you can find a loop in your code changes and do some before/after run time testing and post the results here?

@janosh janosh added performance Some functionality is too slow or regressed linting Linting and quality assurance needs investigation Needs further research to pinpoint cause of issue labels Mar 30, 2024
@DanielYang59
Copy link
Contributor Author

Sure I would have a test and let you know (happy to revert these if contextlib is significantly slower). I'm currently double-checking the re pattern for identify missing raise keywords (as I did notice some)

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Mar 30, 2024

Could confirm contextlib is "significantly" (4x time) slower than try/except:

import timeit

common_setup_code = """
import contextlib
test_list = [0, 1, 2]
"""

method1_code = """
try:
    test_list[10]
except IndexError:
    pass
"""

method2_code = """
with contextlib.suppress(IndexError):
    test_list[10]
"""

# Execute method 1 and measure runtime
method1_time = timeit.timeit(stmt=method1_code, setup=common_setup_code, number=100000)

# Execute method 2 and measure runtime
method2_time = timeit.timeit(stmt=method2_code, setup=common_setup_code, number=100000)

print("Method 1 (except block) time:", method1_time)
print("Method 2 (contextlib.suppress) time:", method2_time)

Gives:

Method 1 (except block) time: 0.0103234
Method 2 (contextlib.suppress) time: 0.040782500000000006

@janosh
Copy link
Member

janosh commented Mar 30, 2024

Could confirm contextlib is "significantly" (4x time) slower than try/except:

probably doesn't matter given we're talking about microseconds here but i'm still a bit ambivalent about this change. also, a slightly cleaner test would be to not include the import contextlib for try/except since it's not needed there and to only import it once in the contextlib case since it's then cached for the rest of the Python session

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Mar 30, 2024

probably doesn't matter given we're talking about microseconds here but i'm still a bit ambivalent about this change.

I'm quite unsure about this change too. contextlib.suppress is much cleaner though being slower (ms difference in 100000 cycles is not a huge difference I guess?).

also, a slightly cleaner test would be to not include the import contextlib for try/except since it's not needed there and to only import it once in the contextlib case since it's then cached for the rest of the Python session

I actually included the import block for both intentionally such that import would not be the cause of time difference, here is the updated script (5x this time):

import timeit

setup_code = """
test_list = [0, 1, 2]
"""

method1_code = """
try:
    test_list[10]
except IndexError:
    pass
"""

method2_code = """
import contextlib
with contextlib.suppress(IndexError):
    test_list[10]
"""

# Execute method 1 and measure runtime
method1_time = timeit.timeit(stmt=method1_code, setup=setup_code, number=100000)

# Execute method 2 and measure runtime
method2_time = timeit.timeit(stmt=method2_code, setup=setup_code, number=100000)

print("Method 1 (except block) time:", method1_time)
print("Method 2 (contextlib.suppress) time:", method2_time)

gives:

Method 1 (except block) time: 0.009449200000000001
Method 2 (contextlib.suppress) time: 0.0490635000000000

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Mar 30, 2024

Well I guess it's better to focus on changes "we really need" at this moment (like type annotation and bug fixes)? I would revert this until we really need it.

@DanielYang59 DanielYang59 changed the title Suppress Exception with contextlib.suppress Replace general raise Exception and add missing raise keyword Mar 30, 2024
@DanielYang59
Copy link
Contributor Author

@coderabbitai review

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Review Status

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 0c20f19 and 12f1585.
Files selected for processing (9)
  • pymatgen/analysis/chemenv/coordination_environments/coordination_geometries.py (1 hunks)
  • pymatgen/analysis/molecule_matcher.py (1 hunks)
  • pymatgen/apps/battery/battery_abc.py (1 hunks)
  • pymatgen/core/operations.py (2 hunks)
  • pymatgen/electronic_structure/bandstructure.py (1 hunks)
  • pymatgen/io/adf.py (1 hunks)
  • pymatgen/io/cif.py (2 hunks)
  • pymatgen/io/vasp/outputs.py (10 hunks)
  • pymatgen/transformations/advanced_transformations.py (2 hunks)
Additional Context Used
Additional comments not posted (26)
pymatgen/apps/battery/battery_abc.py (1)

230-230: The addition of the raise keyword before NotImplementedError is correct and necessary. This ensures that the exception is properly thrown, indicating that the method must be implemented in subclasses.

pymatgen/core/operations.py (2)

487-490: The validation of the time_reversal parameter in the __init__ method is correctly implemented, ensuring that only -1 or 1 are accepted. This is an important step for input validation and prevents potential errors in the use of the MagSymmOp class.


596-596: Replacing the generic Exception with a RuntimeError in the from_xyzt_str method improves error specificity and clarity when the time reversal operator cannot be parsed. This is a good practice in error handling.

pymatgen/io/adf.py (1)

379-379: The change from a generic Exception to a more specific KeyError with a clear error message is a good practice. It makes the error more understandable and debugging easier.

pymatgen/electronic_structure/bandstructure.py (1)

217-217: Replacing the generic Exception with RuntimeError in the conditional check related to projections and structure object presence is a good practice. It provides more specific information about the error, aiding in debugging and maintenance. This change aligns with the PR objectives to enhance error messaging.

pymatgen/analysis/molecule_matcher.py (1)

541-541: The change from a general Exception to a RuntimeError with a specific error message is a good practice. It enhances the clarity of the error, making it easier for developers to understand the issue without diving deep into the code. This change aligns with the PR's objective to improve error handling by using more specific exceptions.

pymatgen/analysis/chemenv/coordination_environments/coordination_geometries.py (1)

1192-1192: The use of RuntimeError here is an improvement over a generic Exception as it provides more specific information about the type of error that occurred. This change aligns with the PR's objective to enhance error handling by using more descriptive exceptions.

pymatgen/io/cif.py (4)

823-823: The deprecation of get_structures and the guidance to use parse_structures instead are clear and well-documented.


823-823: The implementation of parse_structures method is comprehensive, correctly handling various aspects of CIF parsing and structure generation.


823-823: In the CifWriter class, the handling of magnetic moments (write_magmoms) and the option to write site properties (write_site_properties) are well-implemented features that enhance the flexibility of CIF file generation.


823-823: The str2float utility function is robust and handles uncertainties and special cases appropriately.

pymatgen/transformations/advanced_transformations.py (15)

772-778: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [93-111]

The ChargeBalanceTransformation class is well-documented and provides a clear interface for balancing the charge of a structure. Ensure that the apply_transformation method correctly handles edge cases, such as when removal_fraction is negative or when num_in_structure is zero.


772-778: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [113-142]

The SuperTransformation class provides a mechanism to apply multiple transformations to a structure. Ensure that the apply_transformation method properly handles the case when return_ranked_list is not an integer or False, as this could lead to unexpected behavior.


772-778: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [144-204]

The MultipleSubstitutionTransformation class is designed to perform multiple fractional substitutions on a structure. Ensure that the apply_transformation method accurately handles the substitutions and charge balancing, especially in cases where charge_balance_species is specified.


772-778: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [206-375]

The EnumerateStructureTransformation class aims to order disordered structures using enumlib. Ensure that the apply_transformation method correctly handles both ordered and disordered structures, and that it properly manages the enumeration process, especially in terms of performance and accuracy.


772-778: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [377-423]

The SubstitutionPredictorTransformation class uses the structure prediction module to find likely site substitutions. Ensure that the apply_transformation method accurately predicts substitutions and applies them to the structure, particularly in terms of maintaining the structure's stability and physical plausibility.


772-778: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [425-774]

The MagOrderingTransformation class is designed to generate collinear magnetic orderings from a structure. Ensure that the apply_transformation method correctly handles the magnetic ordering process, especially in terms of selecting appropriate magnetic species and managing the enumeration process for different magnetic configurations.


787-793: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [776-802]

The find_codopant function aims to find a suitable codopant based on ionic radius and oxidation state. Ensure that it properly handles cases where no suitable codopant is found or the target species has no defined ionic radius. Consider adding error handling or fallback mechanisms for these scenarios.


772-778: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [804-918]

The DopingTransformation class is designed to perform doping of a structure, with an option for codoping to maintain charge neutrality. Ensure that the apply_transformation method accurately handles the doping process, especially in terms of selecting appropriate doping sites and managing charge balancing.


772-778: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [920-961]

The SlabTransformation class provides a mechanism to create a slab from a bulk structure. Ensure that the apply_transformation method correctly handles the slab creation process, especially in terms of generating slabs with the specified orientation, vacuum thickness, and centering options.


772-778: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [963-1001]

The DisorderOrderedTransformation class aims to generate a disordered structure from an ordered input structure. Ensure that the apply_transformation method accurately generates disordered structures, especially in terms of selecting appropriate sites for disordering and maintaining the physical plausibility of the resulting structure.


772-778: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1003-1034]

The GrainBoundaryTransformation class provides a mechanism to create a grain boundary from a bulk structure. Ensure that the apply_transformation method correctly handles the GB creation process, especially in terms of selecting appropriate rotation axes, angles, and managing the expansion of the grain to avoid interaction between GBs in the cell.


772-778: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1036-1075]

The CubicSupercellTransformation class aims to generate a nearly cubic supercell structure. Ensure that the apply_transformation method accurately generates cubic supercells, especially in terms of scaling the input structure and handling edge cases where the desired cubic dimensions cannot be achieved.


772-778: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1077-1105]

The AddAdsorbateTransformation class is designed to add an adsorbate to a slab structure. Ensure that the apply_transformation method accurately adds the adsorbate to the structure, especially in terms of selecting appropriate adsorption sites and managing the orientation and position of the adsorbate.


772-778: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1107-1135]

The SubstituteSurfaceSiteTransformation class is designed to perform substitution-type doping on the surface of a structure. Ensure that the apply_transformation method accurately performs the substitutions, especially in terms of selecting appropriate surface sites for substitution and managing the substitution process on both surfaces if specified.


772-778: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1137-1165]

The MonteCarloRattleTransformation class uses a Monte Carlo rattle procedure to randomly perturb the sites in a structure. Ensure that the apply_transformation method accurately performs the rattling, especially in terms of managing the minimum interatomic distances and the acceptance probability for each rattle move.

pymatgen/io/cif.py Outdated Show resolved Hide resolved
pymatgen/io/vasp/outputs.py Show resolved Hide resolved
pymatgen/io/vasp/outputs.py Outdated Show resolved Hide resolved
@DanielYang59 DanielYang59 marked this pull request as ready for review March 30, 2024 07:10
@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Mar 30, 2024

@janosh Please review. I'm not 100% confident the proper type of Error was used, please help me double-check. Thanks!

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

thanks @DanielYang59! big fan of more specific error classes. 👍

@janosh janosh removed performance Some functionality is too slow or regressed needs investigation Needs further research to pinpoint cause of issue labels Mar 30, 2024
@janosh janosh added the ux User experience label Mar 30, 2024
@janosh janosh enabled auto-merge (squash) March 30, 2024 07:31
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Status

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 12f1585 and 8b52416.
Files selected for processing (7)
  • pymatgen/analysis/chemenv/coordination_environments/coordination_geometries.py (1 hunks)
  • pymatgen/core/operations.py (2 hunks)
  • pymatgen/electronic_structure/bandstructure.py (1 hunks)
  • pymatgen/io/adf.py (3 hunks)
  • pymatgen/io/cif.py (2 hunks)
  • pymatgen/io/vasp/outputs.py (10 hunks)
  • pymatgen/transformations/advanced_transformations.py (2 hunks)
Files skipped from review as they are similar to previous changes (7)
  • pymatgen/analysis/chemenv/coordination_environments/coordination_geometries.py
  • pymatgen/core/operations.py
  • pymatgen/electronic_structure/bandstructure.py
  • pymatgen/io/adf.py
  • pymatgen/io/cif.py
  • pymatgen/io/vasp/outputs.py
  • pymatgen/transformations/advanced_transformations.py
Additional Context Used

@janosh janosh merged commit 079c354 into materialsproject:master Mar 30, 2024
22 checks passed
@DanielYang59 DanielYang59 deleted the contextlib branch March 30, 2024 09:16
@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Mar 30, 2024

Thanks for reviewing. There seems to be quite a lot of "catching general exceptions" as well (except Exception), might be good to address this as well.

@janosh janosh added the fix Bug fix PRs label Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix PRs linting Linting and quality assurance ux User experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants