iodata icon indicating copy to clipboard operation
iodata copied to clipboard

Improve treatment of occuptions numbers of restricted orbitals

Open tovrstra opened this issue 3 years ago • 2 comments

Fixes #158.

The following remained as it was:

  • Single MolecularOrbitals class for all kinds of orbitals.
  • Restricted orbitals with integer occupation numbers are assumed to represent a restricted open- or closed-shell wavefunctions. E.g. occupation numbers [2, 2, 1, 0, 0] would still represent a doublet state.

The following has changed:

  • Restricted orbitals with fractional occupation numbers are assumed to be spin-paired or closed-shell, unless specified otherwise. E.g. occupation numbers [2, 2, 1.8. 0.2, 0] are assumed to be spin-paired.

  • It is possible to deviate from the above heuristics be specifying mo.occs_aminusb. Just keep in mind that most file formats cannot handle this case. They essentially rely on the above heuristics. Writing a wavefunction file will normally fail when occs_aminusb is set.

tovrstra avatar Mar 31 '21 05:03 tovrstra

Codecov Report

Attention: Patch coverage is 94.62366% with 10 lines in your changes missing coverage. Please review.

Project coverage is 95.61%. Comparing base (66dd4bd) to head (a9828eb). Report is 189 commits behind head on main.

:exclamation: Current head a9828eb differs from pull request most recent head 883d7d8

Please upload reports for the commit 883d7d8 to get more accurate results.

Files Patch % Lines
iodata/formats/fchk.py 66.66% 1 Missing and 1 partial :warning:
iodata/formats/molden.py 0.00% 1 Missing and 1 partial :warning:
iodata/formats/molekel.py 0.00% 1 Missing and 1 partial :warning:
iodata/formats/wfn.py 0.00% 1 Missing and 1 partial :warning:
iodata/formats/wfx.py 33.33% 1 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #252      +/-   ##
==========================================
- Coverage   95.64%   95.61%   -0.04%     
==========================================
  Files          74       74              
  Lines        8268     8387     +119     
  Branches     1072     1086      +14     
==========================================
+ Hits         7908     8019     +111     
- Misses        165      169       +4     
- Partials      195      199       +4     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 31 '21 05:03 codecov[bot]

@FarnazH @PaulWAyers It should be ready for review now. The few remaining coverage misses are fairly harmless.

tovrstra avatar Apr 01 '21 10:04 tovrstra

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

sonarqubecloud[bot] avatar Jun 03 '24 08:06 sonarqubecloud[bot]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Here's the code health analysis summary for commits 7c221cb..883d7d8. View details on DeepSource ↗.

Analysis Summary

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

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

deepsource-io[bot] avatar Jun 05 '24 09:06 deepsource-io[bot]

This is one of the changes that should be done before 1.0.0 because it changes the API. I've brought this zombie-PR back to live by merging the main branch into this PR, so the PR is up-to-date with the recent tooling. (Deepsource complains about complexity of functions, but this problem is not due to the current PR.)

@FarnazH @PaulWAyers It would be good to have your comments on this one before it gets merged.

tovrstra avatar Jun 05 '24 09:06 tovrstra

Thanks for the comments. I'll look up the reason for why some formats are not supporting aminusb, if any. (If there is a good reason, it should be explained better in the comments.)

tovrstra avatar Jun 05 '24 16:06 tovrstra

@PaulWAyers I took a closer look at the code to see how we can address your concern. In the dump_one functions, when occs_aminusb is set, we could convert the IOData instance from a restricted to unrestricted set of orbitals. After that conversion, everything can proceed as normal. The conversion is needed because WFX, WFN, MOLEKEL and MOLDEN formats do not support unrestricted occupation numbers with restricted orbitals. (FCHK does.)

There are a few downsides to such a conversion (at this stage):

  • The resulting files contain a lot of redundant information. At the very least, there should be warning, so users are made aware of this.
  • In general, we don't always do such conversion automatically, to avoid losing the original representation of the data upon writing a file. For this reason, I've created issue #191.

My suggestion would be to not add the conversion at this stage and defer it until there is a proper general solution in place as discussed in #191. (I just edited that issue for clarity.)

tovrstra avatar Jun 06 '24 07:06 tovrstra

Thanks for the review! I'll merge this one and add a comment on the aminusb situation to #191. We have a good number of stale issues, but I love cooking with stale bread. :)

tovrstra avatar Jun 06 '24 15:06 tovrstra