Closed logsdail closed 1 month ago
Attention: Patch coverage is 97.93103%
with 3 lines
in your changes missing coverage. Please review.
Project coverage is 89.38%. Comparing base (
5e7334c
) to head (68e195b
).
Files | Patch % | Lines |
---|---|---|
carmm/run/aims_calculator.py | 94.11% | 1 Missing :warning: |
examples/run_aims.py | 66.66% | 1 Missing :warning: |
examples/utils_python_env_check.py | 96.96% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Noting here that the docs runs with Python 3.9 - should we standardise to this version, perhaps?
So I think after discussion today at a Group meeting, this has become an "update Python version to the most suitable for our HPC architectures".
Current Python compatibility of code: Python 3.7 (i.e., CI tests work to this version)
Current HPC system Python provisions: Hawk: 3.7 (used currently, default), 3.9.2, 3.10.4, 3.11.2 Isambard: 3.7.3.2 (used currently), 3.8.5.1 (default) Young: 3.6 (used currently), 3.7.0, 3.7.2, 3.7.4, 3.8.0, 3.8.6 (default), 3.9.0, 3.9.1, 3.9.6, 3.9.10, 3.11.3, 3.11.4 ARCHER2: 3.6.15 (used currently) 3.9.13.1 (default) 3.10.10
So I think after discussion today at a Group meeting, this has become an "update Python version to the most suitable for our HPC architectures".
Current Python compatibility of code: Python 3.7 (i.e., CI tests work to this version)
Current HPC system Python provisions: Hawk: 3.7 (used currently, default), 3.9.2, 3.10.4, 3.11.2 Isambard: 3.7.3.2 (used currently), 3.8.5.1 (default) Young: 3.6 (used currently), 3.7.0, 3.7.2, 3.7.4, 3.8.0, 3.8.6 (default), 3.9.0, 3.9.1, 3.9.6, 3.9.10, 3.11.3, 3.11.4 ARCHER2: 3.6.15 (used currently) 3.9.13.1 (default) 3.10.10
My take home from this is the minimum we should go to is 3.8 or 3.9, but certainly not towards the high numbers.
@PavelStishenko as suggested I've put a tag on the mainline version, and once merged we'll bumpy the minor version number.
I suggest a second review before merge.
@ikowalec thanks for comments - all addressed. Does someone else want to do a second review, please?
@GaryLZW I simplified the analyse_counterpoise
test, as the checking for ASE now happens in get_aims_calculator
. The CI tests pass but you might want to check if the code works correctly for further use cases.
@GaryLZW I simplified the
analyse_counterpoise
test, as the checking for ASE now happens inget_aims_calculator
. The CI tests pass but you might want to check if the code works correctly for further use cases.
This looks fine to me^^
During my project the following changes were made: The err_handler folder was removed and a utils folder was created in its place A python_env_check file was created with python_env_check() and is_env_python() inside Implemented python_env_check() in anylyse_forces example Implemented python_env_check() in analyse_calculator Restricted mace-torch to 0.3.4, because it was failing on v3.7 Updated CatLearn to the most recent version to fix import error in mlneb.py, by getting the version of CatLearn directly from Git
Thinks still to do after the project: In python v3.8 ci-test there are 4 places where .BadConfiguration Error occurs. Presumably it is the same type of problem that can be fixed in one go Errors: ERROR analyse_counterpoise.py - ase.calculators.calculator.BadConfiguration: No configuration of aims ERROR run_aims.py - ase.calculators.calculator.BadConfiguration: No configuration of aims ERROR run_workflows_ReactAims.py - ase.calculators.calculator.BadConfiguration: No configuration of aims ERROR run_workflows_ReactAims_parallel.py - ase.calculators.calculator.BadConfiguration: No configuration of aims
[like] Andrew Logsdail reacted to your message:
From: FirasAssa @.> Sent: Sunday, August 4, 2024 9:13:06 PM To: logsdail/carmm @.> Cc: Andrew Logsdail @.>; State change @.> Subject: Re: [logsdail/carmm] 30 update codebase to work with Python 3.8 (and thus ase v3.23) (PR #163)
External email to Cardiff University - Take care when replying/opening attachments or links. Nid ebost mewnol o Brifysgol Caerdydd yw hwn - Cymerwch ofal wrth ateb/agor atodiadau neu ddolenni.
During my project the following changes were made: The err_handler folder was removed and a utils folder was created in its place A python_env_check file was created with python_env_check() and is_env_python() inside Implemented python_env_check() in anylyse_forces example Implemented python_env_check() in analyse_calculator Restricted mace-torch to 0.3.4, because it was failing on v3.7 Updated CatLearn to the most recent version to fix import error in mlneb.py, by getting the version of CatLearn directly from Git
Thinks still to do after the project: In python v3.8 ci-test there are 4 places where .BadConfiguration Error occurs. Presumably it is the same type of problem that can be fixed in one go Errors: ERROR analyse_counterpoise.py - ase.calculators.calculator.BadConfiguration: No configuration of aims ERROR run_aims.py - ase.calculators.calculator.BadConfiguration: No configuration of aims ERROR run_workflows_ReactAims.py - ase.calculators.calculator.BadConfiguration: No configuration of aims ERROR run_workflows_ReactAims_parallel.py - ase.calculators.calculator.BadConfiguration: No configuration of aims
— Reply to this email directly, view it on GitHubhttps://github.com/logsdail/carmm/pull/163#issuecomment-2267678961, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFMK4MYUDKHIE4IDSR2UDILZP2KOFAVCNFSM6AAAAABIWCQXZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRXGY3TQOJWGE. You are receiving this because you modified the open/close state.Message ID: @.***>
Testing to updated codebase with ASE release