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

Add prepare_segmented function #373

Merged
merged 2 commits into from
Jul 11, 2024
Merged

Conversation

tovrstra
Copy link
Member

@tovrstra tovrstra commented Jul 11, 2024

This addresses another task from #191: conversion to segmented basis functions is now also implemented through in the prepare and convert modules. In addition to reorganizing how this was done previously, the FCHK format now also makes use of this feature. (For FCHK, SP shells are left in place, but other types of generalized contractions are converted.) This also addresses the last point in #256 .

This is API breaking because the MolecularBasis.get_segmented method was removed and replaced by functions in separate modules. This improves overall modularity: separating utilities from the core data structures. Related unit tests were also moved for consistency.

⚠️ I will YOLO-merge this on Thursday, July 18, unless reviewed earlier.

Summary by Sourcery

This pull request introduces the prepare_segmented function to convert generalized contractions to segmented ones, enhancing modularity by moving this functionality to separate modules. It also updates various format modules to utilize this new feature and refactors related tests for consistency.

  • New Features:
    • Introduced the prepare_segmented function to convert generalized contractions to segmented ones in the prepare module.
    • Added convert_to_segmented function in the convert module for converting basis sets with generalized contractions to single contractions.
  • Enhancements:
    • Removed the MolecularBasis.get_segmented method and replaced it with functions in separate modules to improve modularity.
    • Updated the FCHK format to utilize the new segmented basis conversion feature.
    • Modified the prepare_dump functions in various format modules (Molden, Molekel, WFN, WFX) to include calls to prepare_segmented.
    • Refactored tests to use the new prepare_segmented and convert_to_segmented functions, ensuring consistency and modularity.
  • Tests:
    • Added unit tests for the new prepare_segmented function in test_prepare.py.
    • Added unit tests for the new convert_to_segmented function in test_convert.py.
    • Updated existing tests to remove references to the deprecated MolecularBasis.get_segmented method and replace them with the new functions.

@tovrstra tovrstra added the API breaking Should be done first to stabilize API label Jul 11, 2024
@tovrstra tovrstra added this to the 1.0.0 milestone Jul 11, 2024
Copy link
Contributor

sourcery-ai bot commented Jul 11, 2024

Reviewer's Guide by Sourcery

This pull request introduces the prepare_segmented function to convert generalized contractions to segmented ones, enhancing modularity by moving the MolecularBasis.get_segmented method to separate modules. The changes affect multiple files, including the addition of new tests and modifications to existing ones to ensure compatibility with the new function. The FCHK format now also utilizes this feature, and related unit tests have been updated accordingly.

File-Level Changes

Files Changes
iodata/formats/molden.py
iodata/formats/molekel.py
iodata/formats/wfn.py
iodata/formats/wfx.py
iodata/formats/fchk.py
Updated prepare_dump and dump_one functions to handle segmented basis and call prepare_segmented.
iodata/test/test_convert.py
iodata/test/test_prepare.py
iodata/test/test_basis.py
iodata/test/test_molden.py
iodata/test/test_molekel.py
iodata/test/test_wfx.py
iodata/test/test_fchk.py
iodata/test/test_wfn.py
Added new tests for prepare_segmented and convert_to_segmented functions, and updated existing tests to use convert_to_segmented instead of get_segmented.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link

deepsource-io bot commented Jul 11, 2024

Here's the code health analysis summary for commits cdb30cc..62dc5dd. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource Shell LogoShell✅ SuccessView Check ↗
DeepSource Python LogoPython✅ Success
❗ 1 occurence introduced
🎯 2 occurences resolved
View Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @tovrstra - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 8 issues found
  • 🟢 Security: all looks good
  • 🟡 Testing: 8 issues found
  • 🟡 Complexity: 2 issues found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

iodata/prepare.py Show resolved Hide resolved
iodata/prepare.py Outdated Show resolved Hide resolved
iodata/formats/wfn.py Show resolved Hide resolved
iodata/formats/wfx.py Show resolved Hide resolved
iodata/formats/fchk.py Show resolved Hide resolved
iodata/test/test_molekel.py Show resolved Hide resolved
iodata/test/test_wfn.py Show resolved Hide resolved
iodata/test/test_fchk.py Show resolved Hide resolved
iodata/convert.py Show resolved Hide resolved
iodata/prepare.py Show resolved Hide resolved
Copy link
Member

@PaulWAyers PaulWAyers left a comment

Choose a reason for hiding this comment

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

It seems clear to me.

@tovrstra
Copy link
Member Author

Thanks for checking Paul! Let's see if merging works from a phone...

@tovrstra tovrstra merged commit 4dceda6 into theochem:main Jul 11, 2024
12 checks passed
@tovrstra tovrstra deleted the prepare-segmented branch July 11, 2024 15:06
@FarnazH
Copy link
Member

FarnazH commented Aug 1, 2024

@tovrstra This API change breaks the from_iodata function of gbasis. I have reported this at theochem/gbasis#195.

@PaulWAyers
Copy link
Member

Sorry @FarnazH , I should have caught this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API breaking Should be done first to stabilize API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarifications and documentation improvements in iodata.basis
3 participants