Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modified MRCI implementation in Molpro and Orca adapters #746

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

alongd
Copy link
Member

@alongd alongd commented Jun 20, 2024

Also changed input file memory to be total instead of per CUP
Added test

Currently testing on the server, do not merge, but comments are welcomed

@alongd alongd requested review from kfir4444 and MichalKesl June 20, 2024 12:32
@alongd alongd force-pushed the mrci branch 2 times, most recently from 1ddea11 to 337468e Compare June 25, 2024 07:13
Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 85.10638% with 28 lines in your changes missing coverage. Please review.

Project coverage is 74.26%. Comparing base (fb16f4a) to head (2876e24).

Files with missing lines Patch % Lines
arc/job/adapters/molpro.py 72.72% 6 Missing and 3 partials ⚠️
arc/scheduler.py 0.00% 9 Missing ⚠️
arc/parser.py 87.80% 2 Missing and 3 partials ⚠️
arc/job/adapters/orca.py 84.61% 0 Missing and 2 partials ⚠️
arc/species/species.py 50.00% 1 Missing and 1 partial ⚠️
arc/job/trsh.py 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #746      +/-   ##
==========================================
+ Coverage   74.09%   74.26%   +0.17%     
==========================================
  Files         101      101              
  Lines       28005    28129     +124     
  Branches     5860     5891      +31     
==========================================
+ Hits        20751    20891     +140     
+ Misses       5786     5765      -21     
- Partials     1468     1473       +5     
Flag Coverage Δ
?
unittests 74.26% <85.10%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -37,7 +35,7 @@
settings['output_filenames'], settings['servers'], settings['submit_filenames']

input_template = """***,${label}
memory,${memory},m;
memory,Total=${memory},m;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The also requires a change to the defaults we use in the settings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The memory in settings.py is the max memory the user allowed for troubleshooting purposes (when ARC needs to increase the memory, that is the limit). It shouldn't affect the memory in the input file

Copy link
Collaborator

@kfir4444 kfir4444 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alongd!
Please see the following comments

@alongd
Copy link
Member Author

alongd commented Jun 28, 2024

@kfir4444, I think this PR can be merged. Can you take a final look?

@alongd alongd force-pushed the mrci branch 3 times, most recently from b04591f to 5fbaaba Compare July 2, 2024 19:35
Copy link
Member Author

@alongd alongd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@alongd alongd force-pushed the mrci branch 2 times, most recently from 996253d to eaafcab Compare July 9, 2024 11:28
@alongd alongd force-pushed the mrci branch 2 times, most recently from 69f4ab7 to e3fd2fc Compare August 1, 2024 21:05
@alongd alongd changed the title Modified MRCI implementation in Molpro adapter Modified MRCI implementation in Molpro and Orca adapters Aug 19, 2024
@alongd alongd force-pushed the mrci branch 2 times, most recently from 5c776f5 to 21ef5cc Compare September 14, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants