I think some of these imports are unused, so you could probably remove them.

Also, imports should be grouped by standard imports, third-party libraries, then local imports (PEP 8). It's not that important, but something to consider. For example:

import logging
import os.path
import re
import time

import numpy
import yaml
from yaml import ...

import rmgpy.constants as constants
from rmgpy import ...

Thanks for informing me of the structure, I'll adhere to it.
The imports here are being used implicitly by the eval() function which creates objects from these classes if they are present in the .yml file, so I think we should leave them (otherwise it errors). Since advanced text editors mark them as unused, I think it's a good idea to add a comment next to them explaining why they are kept. I'll happily reorganize imports across Cantherm.

Instead of creating this method, you should modify the getter for Species.molecularWeight.

First, it would be more convenient for me if you could update the syntax to match my PR (this commit). Then, you can change the get method:

@property
def molecularWeight(self):
    if self._molecularWeight is None:
        self._molecularWeight = quantity.Mass(self.molecule[0].getMolecularWeight(), 'kg/mol')
    return self._molecularWeight

Once you do that, you can access the Species.molecularWeight property regardless of whether or not it's been set.

Thanks for the comment!
I'll implement it in the Species __init__():

def __init__(self, ..., molecularWeight=None, ...):
        if self.molecularWeight is None and self.molecule is not None and len(self.molecule) > 0:
            self._molecularWeight = quantity.Mass(self.molecule[0].getMolecularWeight(), 'kg/mol')
        else:
            self.molecularWeight = molecularWeight

Shall I still change the syntax of molecularWeight? I can leave it untouched so #1329 will have less conflicts

On second though, your recommendation seems better if for some reason an empty Species object is first created, then a structure is assigned.

I don't think this should be called species_dictionary. I also think that you could save all of these parameters as normal attributes, then create a method as_dictionary if you need them as a dictionary.

If you do that, I think you also wouldn't need the update method.

I agree.
I think I still need something like the update() method, though, to only append the species attributes to this object (the object is initiated with non-species-specific attributes such as author and level_of_theory which are kept)

Do you know why there are extra blank lines in the adjlist?

I don't know why, it bugs me as well. But I verified that the adjList is being read correctly.
If I simply print it during the run (either using print or logging.info) it looks correct with no extra line brakes.

Some of these parameters don't seem like they would be important to store, e.g. use_atom_corrections, use_bond_corrections, use_hindered_rotors.

At least use_bond_corrections is crucial, the rest are just "nice to have".

use_bond_corrections: This might impact the decision of the user of whether or not to use this file. If the user wants to use this species in a PES with other species calculated at different levels of theory, it is important/recommended to use bond corrections. On the other hand, if all species were calculated at the same level, it is better practice not to use bond corrections (which don't capture all bond types, and are often ambiguous for TSs).

use_hindered_rotors: This could also be directly inferred from the Conformer object. However, for a quick assessment, it's nice to have it explicitly written.

use_atom_corrections: Can be inferred from the atom_energies attribute, I'm OK with removing it since using this feature is less common.

Does this work if the species block comes before the pdep block?

Also, if you refactor the molecular weight implementation based on my other comment, would that eliminate the need for this if block?

modified

It doesn't seem like you use jobList in this function.

removed

This section seems oddly coded. Was it done intentionally? It seems like the following would be sufficient:

number = job.species.conformer.number.value_si.tolist()
coordinates = job.species.conformer.coordinates.value_si.tolist()

Yes, the issue here is that we must pass a np.ndarray type to Molecule().fromXYZ()
The data generated from .tolist() throws an error.
I'll be happy to consider a different alternative to the non straight forward code I used, though.

Oh, I misinterpreted it, I thought initially that the conformer attributes were np.ndarray objects, and you were converting them into lists.

If the conformer attributes are np.ndarray, it seems you could use them directly. If they're lists, then couldn't you use np.array(list_input) to convert it to an np.ndarray?

It seems like you could just call load_species_from_database_file here instead of main.py, and it would also eliminate the need for the load_species_from_database attribute.

I agree, will change accordingly

I think this may be more appropriate as a method of CanthermSpecies. Let me know what you think.

I agree

A couple comments on naming:

• Class names should be CamelCase, though I think we can make an exception for RMG. I would probably call this RMGObject.
• Module names are ideally one word. Perhaps something like object, base, or common?
• Variable names should be lowercase. I think D should be renamed to something else, like output or even output_dict.

Also, could you end the file with a newline?

Thanks for this addition, @mjohnson541 !
I think that self.__dict__ contains methods as well, and we should first check if not callable(value) before adding entries to the output dictionary.

Hmm, my suggestion might not have been correct, since this now misses the case where a structure is not provided. One possibility:

if molecularWeight is not None:
    spec.molecularWeight = molecularWeight
elif spec.molecularWeight is None and is_pdep(jobList):
    raise ValueError()

There shouldn't be any need to check whether structure exists, because the the Species.molecularWeight property checks if it has a molecule attribute, which will be empty if structure was empty.

This actually isn't necessary either, since this is technically not a python file.

Did you mean the .pyx or the .pxd file?

Both

The original implementation is preferable because it treats the file as an iterator, so only one line is loaded at a time. If you use readlines(), then the entire file has to be loaded into memory.

OK, I think this isn't related to this PR (sorry) so I removed it, we could discuss it at another time (I think it might be necessary for running Arkane through ARC). Thanks for bringing this up, I removed the commit.