Method for calculating rotational constants by berquist · Pull Request #568 · cclib/cclib

berquist · 2018-07-17T02:34:55Z

Additionally,

isotopic masses via periodictable package
center of mass
moment of inertia tensor
principal moments of inertia

Handles #256.

Right now, each method returns multiple arrays, one for each set of units. Another way to do it would be to pass a string with the desired output units ("MHz", "cm-1") and only get back one array. That's probably better.

This also only considers the last set of atomcoords.

langner · 2018-07-17T07:52:58Z

requirements.txt

 biopython ; python_version == '2.7' or python_version >= '3.4'
 numpy
+scipy
+periodictable


We have a periodic table thing in util: https://github.com/cclib/cclib/blob/master/cclib/parser/utils.py#L124

Do you think we should replace that with this package, or standardize on our own version?

Our periodic table is not sufficient, because we need masses, specifically isotopic masses. I am not sure if atommasses, when present, are always isotopic masses. They should be when parsed from a frequency calculation. However I don't believe they're always available.

If there is a better Python package for this sort of data, and a better one for physical constants than scipy, I am open to suggestions.

We can eventually have our periodic table wrap an external package, once we pick an appropriate one, because holding our own atomic masses doesn't sound good.

OK, I'm OK with this choice. periodictable look reasonable.

berquist · 2018-07-17T15:20:40Z

I am interested in @ghutchis's thoughts about what features are desired and if he knows about other packages, since he originally requested this feature.

berquist · 2018-07-21T21:12:43Z

For some physical constants, unyt provides a few. One thing I like about it is that it provides the opportunity to handle units in the future. Something I dislike is the provenance of the units is not as clear as SciPy, which specifically says the CODATA version they originate from.

I cannot find a better atomic mass/isotope library than periodictable that is already in Python. There is an excellent one in DALTON, but we cannot use even a Python translation because it is GPL'ed.

berquist · 2018-07-21T21:34:24Z

periodictable does not work with Python 3.2 due to Unicode strings, but is tested with 3.3.

berquist · 2018-07-28T22:11:48Z

I just rediscovered mendeleev, which may be even better than periodictable.

ATenderholt · 2018-07-30T14:31:03Z

Mendeleev seems nice, but has some pretty heavy dependencies like sqlalchemy:
https://bitbucket.org/lukaszmentel/mendeleev/src/fcbaa655eb406237ae465ccc44436248d4b8d145/setup.py?at=default&fileviewer=file-view-default#setup.py-34.

berquist · 2018-07-31T16:33:54Z

periodictable requires numpy and pyparsing.
mendeleev requires six, python-dateutil, pytz, pandas, colorama, pyfiglet, and sqlalchemy.

I did a quick check and there's a small SQLite3 database (< 400 KB) that comes with the package. periodictable stores all its data in a massive comma-separated string. I have no strong opinion either way.

ATenderholt · 2018-07-31T16:42:41Z

My vote is against mendeleev because it has so many dependencies completely unrelated to cclib. Since it's MIT licensed, couldn't we just take their database, write custom queries with the builtin sqlite module, and distribute it with cclib?

berquist · 2018-07-31T17:29:07Z

I would rather choose one package or the other over taking a piece of one, especially since they are both being actively developed. I don't think that there's anything missing from periodictable, so it could replace our own implementation.

ATenderholt · 2018-07-31T18:04:52Z

Fair point about not taking parts of a package if there are actively developed. I agree with going with periodictable.

langner · 2018-08-05T06:09:01Z

I think adding a new dependency requires a bit more thought, a slightly more prolonged discussion. Is there a way to use our own stuff for this PR and have this discussion outside of the development cycle?

berquist · 2018-08-05T20:26:13Z

Just a reminder that the build is failing because of 3.2; bumping to 3.4 solves this and is not really limited to just using periodictable and scipy.

A temporary solution would be to have the mass-dependent parts of this Method work like the bridges, where methods that do not require the external dependency are still usable. So things like the nuclear repulsion energy calculation would still work. It should already be written this way in the PR.

Is there a way to use our own stuff for this PR

It depends on what you mean by this. We don't have masses, either average ones or the isotopic ones we need.

langner

I do wonder about your thoughts on how far cclib should venture into methods territory. On the one hand, there is some value in providing basic methods based on parsed data. On the other hand, it's good to focus on the core functionality and not dilute cclib's mission.

I'm not sure where to draw this line... any thoughts?

langner · 2018-08-09T23:32:37Z

cclib/method/nuclear.py

                nre += zi*zj/d
        return nre
+
+    @staticmethod


Why shouldn't this be a module-level function?

langner · 2018-08-09T23:33:07Z

cclib/method/nuclear.py

+                abundance = iso.abundance
+        return most_abundant_isotope
+
+    @staticmethod


langner · 2018-08-09T23:34:57Z

requirements.txt

 biopython ; python_version == '2.7' or python_version >= '3.4'
 numpy
+scipy
+periodictable


OK, I'm OK with this choice. periodictable look reasonable.

berquist · 2018-08-14T03:12:50Z

The advantage to having all these methods is that they live right next to data type they take as input, and are an example of what you can do with cclib. The hope is that you don't need to always reinvent the wheel when building an analysis on top of ccData objects.

It's nice that using the parser and readers/writers doesn't require using the methods. However, going the other direction, having the readers and writers close by means that you can take just an XYZ file and calculate the nuclear repulsion energy or rotational constants, for example.

An option is to split off all the methods into their own repository. My feeling is that many people don't know about or use the methods as it is, and moving them somewhere else would make that worse.

I think that we should allow any method that doesn't require more than attributes and physical constants, as long as its correctness can be verified.

langner · 2018-08-14T15:31:06Z

Thanks for the thoughts, my opinion is roughly the same. Added this topic to #555 which should ideally go into docs for developers.

langner · 2018-08-14T15:34:13Z

cclib/method/nuclear.py



+def get_most_abundant_isotope(element):
+    """Given a `periodictable` element, return the most abundant


I'm not a fan of strictly keeping to the 80 column limit. Having slightly longer lines so that they don't wrap in a n ugly way is fine IMO, as long as the length is reasonable. What do you think? We could also cover this in #555.

I think I'm in agreement; I have a habit of running fill-paragraph (or your editor's equivalent, gq?) for docstrings, and the default column width is 70. I can stop doing that.

Yeah, let's have a convention to use only one tool if we want to auto-format. We have enough pep8 violations that it would make sense to format in bulk once if we want to go in that direction.

langner · 2018-08-14T15:34:31Z

cclib/method/nuclear.py

+    """Return the masses for the given nuclei, respresented by their
+    nuclear charges.
+    """
+    import periodictable


Shouldn't we import (softly) at top of module?

Yes, I'll make it consistent with the bridges for now.

langner · 2018-08-26T18:43:03Z

I think this is ready to go, and the Travis failure is for 3.2... @berquist probably need to rebase to get it tested in 3.4 (that was merged a moment ago).

Additionally, * isotopic masses via `periodictable` package * center of mass * moment of inertia tensor * principal moments of inertia

berquist added feature methods labels Jul 17, 2018

berquist added this to the v1.5.4 milestone Jul 17, 2018

langner reviewed Jul 17, 2018

View reviewed changes

berquist mentioned this pull request Jul 28, 2018

Drop Python 3.2 and bump Travis to 3.4 #577

Closed

langner requested changes Aug 9, 2018

View reviewed changes

berquist force-pushed the rotational-constants branch from 5a34f02 to b1e6265 Compare August 14, 2018 03:18

langner requested changes Aug 14, 2018

View reviewed changes

langner approved these changes Aug 26, 2018

View reviewed changes

berquist added 6 commits August 26, 2018 15:26

Method for calculating rotational constants

97c201c

Additionally, * isotopic masses via `periodictable` package * center of mass * moment of inertia tensor * principal moments of inertia

Determine rotational constant units from method argument.

d3fc3a3

Determine units for principal moments from method argument.

6bba695

Nuclear methods depending on atomcoords should allow any geometry.

7cd8789

Nuclear: turn classmethods into module functions

92db826

Nuclear: make dependency import consistent with bridges

3c1ce8e

berquist force-pushed the rotational-constants branch from d1f3086 to 3c1ce8e Compare August 26, 2018 19:28

This was referenced Aug 26, 2018

Replace parser.utils.PeriodicTable with periodictable #604

Closed

Developer guidelines #555

Open

langner merged commit a39fa1e into cclib:master Sep 1, 2018

berquist deleted the rotational-constants branch September 1, 2018 20:00

berquist mentioned this pull request Sep 2, 2018

Support for rotational constants #256

Closed

berquist mentioned this pull request Mar 11, 2024

Add and update developer guidelines #1355

Merged

23 tasks



		def get_most_abundant_isotope(element):
		"""Given a `periodictable` element, return the most abundant

Conversation

berquist commented Jul 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berquist commented Jul 17, 2018

Uh oh!

berquist commented Jul 21, 2018

Uh oh!

berquist commented Jul 21, 2018

Uh oh!

berquist commented Jul 28, 2018

Uh oh!

ATenderholt commented Jul 30, 2018

Uh oh!

berquist commented Jul 31, 2018

Uh oh!

ATenderholt commented Jul 31, 2018

Uh oh!

berquist commented Jul 31, 2018

Uh oh!

ATenderholt commented Jul 31, 2018

Uh oh!

langner commented Aug 5, 2018

Uh oh!

berquist commented Aug 5, 2018

Uh oh!

langner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berquist commented Aug 14, 2018

Uh oh!

langner commented Aug 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

langner commented Aug 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants