Skip to content

Organize the level of theory representation in ARC#399

Merged
alongd merged 44 commits into
masterfrom
level
Jun 4, 2020
Merged

Organize the level of theory representation in ARC#399
alongd merged 44 commits into
masterfrom
level

Conversation

@alongd

@alongd alongd commented May 26, 2020

Copy link
Copy Markdown
Member

Added a Level class that converts easily to the new Arkane LevelOfTheory class to determine freq scale factor and energy corrections.
This class also organizes the different functions related to the level of theory.

This PR also introduces significant changes to main, resulting in changes to ARC's API. In particular:

  • removed input/restart file load and argument checks, now an input file and the API are indistinguishable in main (removed the from_dict method).
  • non-backward compatible API changes:
    • species (was arc_species_list),
    • reactions (was arc_rxn_list),
    • arkane_level_of_theory (was model_chemistry, now only refers to the sp level, not freq level),
    • removed job_additional_options and job_shortcut_keywords, they are included in the Level object as an args argument
    • removed solvation, now included in the Level object which can be specified per job type
    • thermo_adapter and kinetics_adapter were added, replacing the general statmech_adapter
    • bac_type specifies 'p' or 'm' BAC type (could be None for not using BACs), replaced the boolean flag use_bac
  • changed how level of theories per job type are handled

Comment thread arc/level.py
Comment thread arc/level.py
Comment thread arc/level.py Outdated
Comment thread arc/level.py Outdated
Comment thread arc/level.py
Comment thread arc/level.py Outdated
Comment thread arc/level.py Outdated
@amarkpayne

amarkpayne commented May 26, 2020

Copy link
Copy Markdown
Member

I haven't gotten a chance to look at the code too closely yet, but I am concerned about this implementation easily diverging from the one in RMG-Py. Of course, as you noted there is a problem because the RMG-Py class discards hyphens (it does this so that the methods are not necessarily software dependent).

I wonder if there is a way that we can use the RMG-Py objects but build an interface to them such that issues like hyphens are handled when ARC needs them. This shouldn't be an issue when trying to convert ARC representations to LOT (discarding hyphens is easy), but I think there should be a solution to go the other way as well.

One possible idea: What if we implement adapters between LOT and each quantum chemistry software? Some of these software adapters might be very simple if the software does not care about hyphens.

@cgrambow also has an interesting idea about using the __repr__ method to store things like wb97X-D with hyphens but that allow interfacing with Arkane. I'll let him comment here about it as well.

We should discuss this tomorrow during subgroup if you want, or setup a Zoom meeting

@cgrambow

Copy link
Copy Markdown

Adding on to what @amarkpayne is saying, I also don't think it's necessary to implement yet another class for dealing with levels of theory; rather we could try to make the level of theory classes in RMG-Py suitable for ARC. This is what I was thinking:

  • Implement LevelOfTheory and CompositeLevelOfTheory such that if you pass it, for example, method='wB97X-D', it will store the "non-standardized" string wB97X-D as the method attribute in LevelOfTheory.
  • LevelOfTheory.__repr__ can remain unchanged such that it will create the string representation based on the string that you pass it (e.g., the representation for the above example would be LevelOfTheory(method='wB97X-D')), which allows differentiating versions with/without hyphens by manipulating the attribute values.
  • Re-implement the default __eq__ and __hash__ methods of LevelOfTheory such that they convert attributes to their standardized representations prior to comparisons. For example, this means that LevelOfTheory(method='wB97X-D') == LevelOfTheory(method='wb97xd') would evaluate to True and that LevelOfTheory(method='wB97X-D') and LevelOfTheory(method='wb97xd') are identical keys in dictionaries.

@oscarwumit

oscarwumit commented May 26, 2020

Copy link
Copy Markdown
Contributor

@amarkpayne I think implementing adapters between LOT and each quantum chemistry software is a good idea and a potential way to clean up some long if-else rules in ARC. Also, we should think about within each ESS, how to treat different job types.

Currently, ARC uses if-else to treat job_type and ESS together, which is really hard to maintain.

Maybe to generate a job we can do: LOT --> ESS adaptor --> method for creating the input file for the required job_type for the particular ESS

@cgrambow

Copy link
Copy Markdown

@oscarwumit @alongd Check out the modifications I just pushed to the modify_lot branch (https://github.com/ReactionMechanismGenerator/RMG-Py/tree/modify_lot) and see if they accomplish what you need. Hyphens and spaces are no longer dropped. This only occurs implicitly when comparing level of theory objects.

@alongd

alongd commented May 27, 2020

Copy link
Copy Markdown
Member Author

@cgrambow @amarkpayne , sounds good!
We'll also need to implement additional methods and arguments to LevelOfTheory (see this updated PR):

  • a clean dict representation to save in ARC's restart file
  • a from_dict method
  • solvation_scheme_level - when you do the solvation correction at a different level
  • method_type - specifying whether a method is DFT, WF, MM etc.
  • make args a dict with keyword and block keys
  • A YAML file with supported methods per software

These are all implemented in this PR if you'd like to take a look, I'm happy to relocate them to Arkane.

@alongd

alongd commented May 27, 2020

Copy link
Copy Markdown
Member Author

I take my above comment back: There's no need to add all that stuff to Arkane which isn't being used in the RMG repo, instead, we should set up a class in ARC that inherits from LevelOfTheory (made possible by @cgrambow's recent commit - let's PR it), and augment it with additional arguments and methods that are only used by ARC.

@alongd

alongd commented May 31, 2020

Copy link
Copy Markdown
Member Author

@oscarwumit, could you review the Level class again? Let's talk tomorrow.
@xiaoruiDong, could you take a look at this PR?

There are some API changes, see the edited first comment for details. I plan to modify the examples and scripts as needed, and modify the documentation.

alongd added 25 commits June 3, 2020 15:03
to avoid maintaining two separate argument checks in main
- removed input/restart file load and argument checks
- non backward compatible API changes: species (arc_species_list), reactions (arc_rxn_list), arkane_level_of_theory (model_chemistry), removed job_additional_options and job_shortcut_keywords, removed solvation, thermo_adapter and kinetics_adapter instead of statmech_adapter, bac_type (use_bac)
- changed how level of theories per job type are handled
- removed the from_dict method
- determine_model_chemistry_type
- format_level_of_theory_inputs
- format_level_of_theory_for_logging

other minor style modifications, also blocked matplotlib warnings
Also includes minor style modifications to Scheduler
This is a complementary commit, many modifications were already made in other commits in this PR. Since shebangs are only important for executable files, they were removed from non-executable files, leaving them in ARC.py, scripts, and tests.
@lgtm-com

lgtm-com Bot commented Jun 3, 2020

Copy link
Copy Markdown

This pull request fixes 4 alerts when merging 35ba12b into 2e71a1a - view on LGTM.com

fixed alerts:

  • 2 for Unused import
  • 1 for Unused local variable
  • 1 for Suspicious unused loop iteration variable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants