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

Fix env #652

Merged
merged 5 commits into from
May 14, 2023
Merged

Fix env #652

merged 5 commits into from
May 14, 2023

Conversation

kfir4444
Copy link
Collaborator

@kfir4444 kfir4444 commented May 9, 2023

This PR contains two features:

  1. An update to the ARC environment, specifically numpy and cantera version update, and removal of nosetests, which is deprecated
  2. replacing the @wip decorator with a one that is not dependent on nosetests.

dependencies:
- cairo
- cairocffi
- rmg::cantera >=2.3.0
- cantera::cantera=2.6
Copy link
Member

Choose a reason for hiding this comment

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

maybe >=?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That operator is not generally considered good practice, at his worst this scrambles your dependencies without the developer's knowing, at this best it gives you exactly what you want.

@alongd
Copy link
Member

alongd commented May 9, 2023

Do you think we should bump numpy more? See #517

@kfir4444 kfir4444 force-pushed the fix_env branch 2 times, most recently from 3ef1985 to c7ad81a Compare May 9, 2023 09:13
@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #652 (8d1aac8) into main (a56b0df) will increase coverage by 0.00%.
The diff coverage is 75.00%.

@@           Coverage Diff           @@
##             main     #652   +/-   ##
=======================================
  Coverage   73.12%   73.13%           
=======================================
  Files          99       99           
  Lines       26284    26277    -7     
  Branches     5497     5497           
=======================================
- Hits        19221    19217    -4     
+ Misses       5700     5698    -2     
+ Partials     1363     1362    -1     
Impacted Files Coverage Δ
arc/settings/settings.py 89.01% <ø> (-0.47%) ⬇️
arc/utils/wip.py 90.00% <75.00%> (+5.38%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

arc/plotter.py Outdated
plt.savefig(image_path, bbox_inches='tight')
try:
plt.savefig(image_path, bbox_inches='tight')
except FileNotFoundError:

Check notice

Code scanning / CodeQL

Empty except

'except' clause does nothing but pass and there is no explanatory comment.
This module that contains functional tests for ARC
"""

from encodings import utf_8

Check notice

Code scanning / CodeQL

Unused import

Import of 'utf_8' is not used.
from encodings import utf_8
import os
import shutil
import subprocess

Check notice

Code scanning / CodeQL

Unused import

Import of 'subprocess' is not used.
@@ -36,7 +36,7 @@

from arc.common import get_logger, is_angle_linear, key_by_val, determine_top_group_indices
from arc.exceptions import ZMatError
from arc.species.vectors import calculate_distance, calculate_angle, calculate_dihedral_angle, get_vector_length
from arc.species.vectors import calculate_param, get_vector_length

Check failure

Code scanning / CodeQL

Module-level cyclic import

'calculate_param' may not be defined if module [arc.species.vectors](1) is imported before module [arc.species.zmat](2), as the [definition](3) of calculate_param occurs after the cyclic [import](4) of arc.species.zmat.
@@ -36,7 +36,7 @@

from arc.common import get_logger, is_angle_linear, key_by_val, determine_top_group_indices
from arc.exceptions import ZMatError
from arc.species.vectors import calculate_distance, calculate_angle, calculate_dihedral_angle, get_vector_length
from arc.species.vectors import calculate_param, get_vector_length

Check failure

Code scanning / CodeQL

Module-level cyclic import

'get_vector_length' may not be defined if module [arc.species.vectors](1) is imported before module [arc.species.zmat](2), as the [definition](3) of get_vector_length occurs after the cyclic [import](4) of arc.species.zmat.
"""

import os
import pytest

Check notice

Code scanning / CodeQL

Unused import

Import of 'pytest' is not used.
arc/common.py Outdated
@@ -1618,7 +1622,7 @@
break


def sort_atoms_in_decending_label_order(mol: 'Molecule') -> None:
def sort_atoms_in_descending_label_order(mol: 'Molecule') -> None:

Check warning

Code scanning / CodeQL

Variable defined multiple times

This assignment to 'sort_atoms_in_descending_label_order' is unnecessary as it is [redefined](1) before this value is used.
arc/job/local.py Fixed Show fixed Hide fixed
@kfir4444
Copy link
Collaborator Author

Do you think we should bump numpy more? See #517
Probably not possible for now, we would need to update python:
├─ numpy 1.22.0** is installable with the potential options
│ ├─ numpy 1.22.0 would require
│ │ └─ python >=3.9,<3.10.0a0 , which can be installed;
│ ├─ numpy 1.22.0 would require
│ │ └─ python >=3.10,<3.11.0a0 , which can be installed;
│ └─ numpy 1.22.0 would require
│ └─ python >=3.8,<3.9.0a0 , which can be installed;
└─ python 3.7** is uninstallable because there are no viable options
├─ python [3.7.0|3.7.1|...|3.7.9] conflicts with any installable versions previously reported;
└─ python 3.7.1 would require
└─ openssl >=1.1.1,<1.1.2.0a0 , which does not exist (perhaps a missing channel).

kfir4444 and others added 3 commits May 13, 2023 12:03
This is due to the recent rmg environment update. removed nosetests (deprecated), xTB (not needed) and updated numpy and cantera versions. Also added anaconda channel, for openssl.
It appears we did not fully update which xtb we use. Even though we installed xtb_env (xtb = 6.6.0), we kept using xtb in arc_env (xtb = 6.3.3)
Copy link
Member

@calvinp0 calvinp0 left a comment

Choose a reason for hiding this comment

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

Everything looks good to me with the exception of the minor pedantic removal of certain paths when looking for xtb

arc/settings/settings.py Outdated Show resolved Hide resolved
arc/settings/settings.py Outdated Show resolved Hide resolved
Since we updated numpy on the arc_env, it allowed us inadvertently to update our xtb to 6.6.0 which means we were not writing the correct input file for xtb anymore (STILL TO BE ASSESSED)
Thus, we need to revert our xtb_env to 6.3.3 for the time being
@calvinp0 calvinp0 self-requested a review May 13, 2023 15:36
alongd
alongd previously approved these changes May 13, 2023
Copy link
Member

@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.

Looking good!
I don't support locking dependable versions using a =, I do think we should allow >=
Other than that it's good to go

@alongd
Copy link
Member

alongd commented May 14, 2023

Are we clear to merge?

@calvinp0 calvinp0 merged commit 17f8512 into main May 14, 2023
@calvinp0 calvinp0 deleted the fix_env branch May 14, 2023 05:09
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.

None yet

3 participants