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

Complete prepare_dump API and apply to occs_aminusb + cleanups #352

Merged
merged 5 commits into from
Jun 28, 2024

Conversation

tovrstra
Copy link
Member

@tovrstra tovrstra commented Jun 27, 2024

This is another step in #191, adding a small new feature: the conversion of to unrestricted orbitals when restricted orbitals with occs_aminusb is not supported by the selected file format.

This feature is implemented at two levels:

  • In iodata.orbitals module, a new function convert_to_unrestricted is added, which provides the basic functionality. It is useful also outside the context of writing files. Splitting this off also facilitates testing. This is a function (and not a method) on purpose: the class itself does not need it to fulfill its main role, so it can be kept separate.
  • In the new iodata.prepare module, a function prepare_unrestricted_aminusb is created, building on the previous, adding all the logic for errors, warnings and geared towards prepare_dump functions of the relevant file formats.

Test coverage of the new code should be fairly complete, but I'll check the CI output to be sure.

There are a few related changes (and a few deferred ones, new tasks in #191):

  • Optional arguments in dump_one and dump_many must now be given as keyword arguments for the sake of clarity. The same change for other functions in iodata.api will be made in one of the upcoming PRs.
  • An assertion in the Molekel load_one function has been turned into a warning. (Eventually, we need to replace all asserts in non-test code. This is on the list in Janitoring todo list before 1.0.0 #204) A new test for this warning is included.

I will YOLO-merge this on Friday July 5, unless reviewed earlier.

Summary by Sourcery

This pull request introduces a new feature to convert restricted orbitals to unrestricted orbitals and integrates this functionality into the prepare_dump process for various file formats. It also includes enhancements to the API for clarity, a bug fix for spin polarization handling, and comprehensive test coverage for the new functionality.

  • New Features:
    • Added a new function convert_to_unrestricted in the iodata.orbitals module to convert restricted orbitals to unrestricted orbitals.
    • Introduced a new prepare_unrestricted_aminusb function in the iodata.prepare module to handle conversion logic for prepare_dump functions.
  • Bug Fixes:
    • Changed an assertion in the Molekel load_one function to a warning to handle inconsistencies in spin polarization.
  • Enhancements:
    • Updated dump_one and dump_many functions in iodata.api to require optional arguments as keyword arguments for clarity.
    • Modified prepare_dump functions in various format modules (Molekel, Molden, WFN, WFX, FCHK, JSON) to use the new prepare_unrestricted_aminusb function for handling occs_aminusb.
  • Tests:
    • Added comprehensive test coverage for the new convert_to_unrestricted function in iodata/test/test_orbitals.py.
    • Included new tests for the prepare_unrestricted_aminusb function in iodata/test/test_prepare.py.
    • Added a test in iodata/test/test_molekel.py to check for warnings when loading files with incorrect spin multiplicity.

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

sourcery-ai bot commented Jun 27, 2024

Reviewer's Guide by Sourcery

This pull request introduces a new feature to convert restricted orbitals to unrestricted when occs_aminusb is set, implemented via a new function convert_to_unrestricted in the iodata.orbitals module and a new prepare_unrestricted_aminusb function in the iodata.prepare module. It also includes updates to the prepare_dump functions across various file formats to utilize this new functionality, and modifies dump_one and dump_many to include an allow_changes parameter. Additionally, new tests have been added to ensure comprehensive test coverage.

File-Level Changes

Files Changes
iodata/formats/molekel.py
iodata/formats/molden.py
iodata/formats/wfn.py
iodata/formats/wfx.py
Updated prepare_dump functions to use prepare_unrestricted_aminusb for handling occs_aminusb.
iodata/api.py
iodata/__main__.py
Modified functions to use keyword arguments for clarity and added allow_changes parameter.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • 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 Jun 27, 2024

Here's the code health analysis summary for commits e4efb2e..565cfdf. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource Shell LogoShell✅ SuccessView Check ↗
DeepSource Python LogoPython✅ Success
❗ 3 occurences 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: 3 issues found
  • 🟢 Security: all looks good
  • 🟡 Testing: 3 issues found
  • 🟡 Complexity: 3 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/api.py Outdated Show resolved Hide resolved
iodata/formats/molden.py Show resolved Hide resolved
iodata/__main__.py Outdated Show resolved Hide resolved
iodata/test/test_orbitals.py Show resolved Hide resolved
iodata/test/test_orbitals.py Show resolved Hide resolved
iodata/test/test_prepare.py Show resolved Hide resolved
iodata/formats/fchk.py Show resolved Hide resolved
iodata/formats/molden.py Show resolved Hide resolved
iodata/orbitals.py Show resolved Hide resolved
@tovrstra
Copy link
Member Author

The deepsource issues can be ignored:

  • Unused arguments are legit in this case for the sake of API consistency. Deepsource doesn't seem to realize. It requires a more global analysis of the code, not just checking patterns, to see that this is ok.
  • The complexity of the molekel load_one function was already present before this PR.

@tovrstra
Copy link
Member Author

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.

Thanks Toon, I really think this improves the usability.

@tovrstra
Copy link
Member Author

Thanks for reviewing, Paul. Merging...

@tovrstra tovrstra merged commit 03bf3e6 into theochem:main Jun 28, 2024
12 checks passed
@tovrstra tovrstra deleted the prepare-unrestricted branch June 28, 2024 07:56
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.

2 participants