Fix "unsupported operand" issue when accessing IOData.charge #196

tovrstra · 2020-08-25T08:11:11Z

This is the fix for the following type of error message:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/maarten/anaconda3/envs/iodata/lib/python3.8/site-packages/iodata/iodata.py", line 274, in charge
    return self.atcorenums.sum() - self.nelec
TypeError: unsupported operand type(s) for -: 'float' and 'NoneType

This issue is hampering two PRs (#188 and #186), so I'm putting in a separate PR, which can be merged easily.

(Related issues with nelec and spinpol are also fixed.)

Related issues with nelec and spinpol are also fixed.

FarnazH · 2020-08-25T14:48:17Z

iodata/iodata.py

+        if self._charge is not None:
+            # _charge is treated as the dependent one, while atcorenums and
+            # nelec are treated as independent.
+            if self.nelec is None:
+                # Switch to storing _nelec.
+                self._nelec = atcorenums.sum() - self._charge
+            self._charge = None


I have a naive questions: why in the last self._charge is set to None? Is this because self._charge is only assigned when charge is assigned, and the derived approaches to get charge wouldn't set any of the private attributes (e.g. self._charge)?

If I understood the code correctly, by assigning the variables _atcorenums and _nelec, it is possible to get the net charge value and even update it. If the charge was assigned before setting _atcorenums, it remains in _charges. By using self._charge = None, we get rid of the initial charge.

I think I intended it the way Luis explained. It is just to clarify (thus not strictly necessary) that the charge is going to be derived from nelec and atcorenums.

FarnazH · 2020-08-25T14:51:48Z

iodata/iodata.py

            result = self.atnums.astype(float)
-            self._atcorenums = result
+            self.atcorenums = result


Picky Comment: Can we get rid of result variable? The same at the beginning of if statement, I couldn't add a comment there.

(Comments are only possible on lines that have changed, which is a bit unfortunate, because sometimes new changes imply things about old code.)

Agreed, result can be eliminated, making things shorter. I'm not sure why it was this way, probably related to some old code.

FarnazH

I am still going through this to fully understand it, but what about this case:

from iodata import IOData

d = IOData()
d.charge = 0
d.atcorenums = None

This gives the error below:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/Farnaz/packages/iodata/iodata/iodata.py", line 274, in atcorenums
    self._nelec = atcorenums.sum() - self._charge
AttributeError: 'NoneType' object has no attribute 'sum'

lmacaya · 2020-08-26T03:20:31Z

iodata/iodata.py

+        if self._charge is not None:
+            # _charge is treated as the dependent one, while atcorenums and
+            # nelec are treated as independent.
+            if self.nelec is None:


Suggested change

if self.nelec is None:

if self.nelec is None and atcorenums is not None:

Maybe this fix AttributeError: 'NoneType' object has no attribute 'sum'

To get intuitive behavior, the code had to be extended, which is now the case in the latest commit. I've added several variations of unit tests to make sure it works (and checked with coverage that they reached all code paths.)

lmacaya · 2020-08-26T03:37:29Z

iodata/iodata.py

    def charge(self, charge: float):
+        # The internal _charge is used only if atcorenums is None.
        if self.atcorenums is None:
            self._charge = charge
        else:


If data.charge is set to None, after setting data.atcorenums and data.charge, raise an error (Line 291):

import numpy as np from iodata import IOData d = IOData() d.atnums = np.array([6, 1, 1, 1, 1]) d.charge = 1 d.charge = None

Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/lmacaya/Desktop/horton3/fix_charge_issue/iodata/iodata/iodata.py", line 291, in charge self.nelec = self.atcorenums.sum() - charge TypeError: unsupported operand type(s) for -: 'float' and 'NoneType'

I don't know if this is an issue, but I point it out just in case.

Needs to be fixed. I guess this is comparable to one of @FarnazH 's comments. Thanks for bringing this up.

This is also fixed now.

lmacaya

Looks good! I only noticed one problem in the atcorenum setter, but I think the main issues are covered.

lmacaya · 2020-08-27T14:24:15Z

iodata/iodata.py

+            if self.nelec is not None:
+                # Set _charge because charge can no longer be derived from
+                # atcorenums and nelec.
+                self._charge = self._atcorenums.sum() - self.nelec


I think this line will raise an error if nelec was set before atcorenums and the latter is set to None afterwards.

Thanks. I'll add a unit test for this scenario, and make sure it works.

You were right! It should be fixed now. I also found another minor issue with the dtype of atcorenums, which is fixed too.

mcoolsce

Everything looks good to me, except one possible inconsistency I could find.

mcoolsce · 2020-08-27T17:13:48Z

iodata/iodata.py

-        if self.mo is None:
-            return self._nelec
-        return self.mo.nelec
+        # When the molecular orbitals are present, they determine the number


If a IOData object is created with only charge and atnums present

mol = IOData(charge=-1, atnums=[8, 1, 1]) print(mol.nelec)

printing out nelec results in None while, after accessing atcorenums first, it prints out the correct value of 11.0.

mol = IOData(charge=-1, atnums=[8, 1, 1]) print(mol.atnumcores) print(mol.nelec)

Is this the correct behaviour, or should nelec also be derived from atnums and the charge only?

Good point, and there was a way to fix it. pfew.

FarnazH

Sorry, there was a mistake on my side in the code snippet I shared... I am testing the latest implementation.

FarnazH

I tried a few cases, and the results were as expected. Thanks for all the fixes.

tovrstra · 2020-08-28T04:36:08Z

@FarnazH @lmacaya @mcoolsce Thanks for reviewing everyone! It really helped a lot!

tovrstra assigned FarnazH and lmacaya Aug 25, 2020

tovrstra mentioned this pull request Aug 25, 2020

Input Writer #188

Merged

tovrstra requested review from FarnazH and lmacaya August 25, 2020 08:21

tovrstra unassigned FarnazH and lmacaya Aug 25, 2020

tovrstra marked this pull request as draft August 25, 2020 08:55

tovrstra added the bug label Aug 25, 2020

tovrstra added 2 commits August 25, 2020 11:30

Fix "unsupported operand" issue when accessing IOData.charge

457851f

Related issues with nelec and spinpol are also fixed.

Improvements and more tests

d85e9a9

tovrstra force-pushed the fix_charge_issue branch from 378ba48 to d85e9a9 Compare August 25, 2020 09:30

Add more tests and comments

54c5873

tovrstra marked this pull request as ready for review August 25, 2020 11:14

FarnazH reviewed Aug 25, 2020

View reviewed changes

lmacaya reviewed Aug 26, 2020

View reviewed changes

tovrstra added 2 commits August 26, 2020 15:12

Fix issues brought up in review

714b4aa

Fix duplicate name

0ee5295

tovrstra mentioned this pull request Aug 26, 2020

mwfn load_one #198

Merged

tovrstra requested review from lmacaya and mcoolsce August 27, 2020 11:44

lmacaya reviewed Aug 27, 2020

View reviewed changes

Fix issue found by Luis

5f37b0d

mcoolsce reviewed Aug 27, 2020

View reviewed changes

tovrstra added 3 commits August 27, 2020 20:20

Fix for comment Maarten, consistency in constructor

2975c35

Put pylint exceptions in correct scope

7ea9300

Call all setters of construction

32559be

FarnazH self-requested a review August 27, 2020 20:44

FarnazH reviewed Aug 27, 2020

View reviewed changes

FarnazH self-requested a review August 28, 2020 04:34

FarnazH approved these changes Aug 28, 2020

View reviewed changes

tovrstra merged commit e247a0a into theochem:master Aug 28, 2020

tovrstra mentioned this pull request Aug 28, 2020

Parse extended xyz #186

Merged

tovrstra deleted the fix_charge_issue branch June 5, 2024 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "unsupported operand" issue when accessing IOData.charge #196

Fix "unsupported operand" issue when accessing IOData.charge #196

tovrstra commented Aug 25, 2020

FarnazH Aug 25, 2020

lmacaya Aug 26, 2020 •

edited

Loading

tovrstra Aug 26, 2020

FarnazH Aug 25, 2020

tovrstra Aug 26, 2020

FarnazH left a comment •

edited

Loading

lmacaya Aug 26, 2020

tovrstra Aug 26, 2020

tovrstra Aug 26, 2020

lmacaya Aug 26, 2020 •

edited

Loading

tovrstra Aug 26, 2020

tovrstra Aug 26, 2020

lmacaya left a comment

lmacaya Aug 27, 2020

tovrstra Aug 27, 2020

tovrstra Aug 27, 2020

mcoolsce left a comment •

edited

Loading

mcoolsce Aug 27, 2020

tovrstra Aug 27, 2020

FarnazH left a comment •

edited

Loading

FarnazH left a comment

tovrstra commented Aug 28, 2020

	if self.nelec is None:
	if self.nelec is None and atcorenums is not None:

Fix "unsupported operand" issue when accessing IOData.charge #196

Fix "unsupported operand" issue when accessing IOData.charge #196

Conversation

tovrstra commented Aug 25, 2020

Choose a reason for hiding this comment

lmacaya Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FarnazH left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmacaya Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmacaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcoolsce left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FarnazH left a comment • edited Loading

Choose a reason for hiding this comment

FarnazH left a comment

Choose a reason for hiding this comment

tovrstra commented Aug 28, 2020

lmacaya Aug 26, 2020 •

edited

Loading

FarnazH left a comment •

edited

Loading

lmacaya Aug 26, 2020 •

edited

Loading

mcoolsce left a comment •

edited

Loading

FarnazH left a comment •

edited

Loading