duartegroup / autodE

automated reaction profile generation
https://duartegroup.github.io/autodE/
MIT License
165 stars 51 forks source link

Remove hessian conversion on normal mode calculation #243

Closed t-young31 closed 1 year ago

t-young31 commented 1 year ago

Resolves #239


Checklist

codecov[bot] commented 1 year ago

Codecov Report

Merging #243 (0ab0d2c) into v1.3.5 (0d03666) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           v1.3.5     #243      +/-   ##
==========================================
+ Coverage   97.14%   97.16%   +0.01%     
==========================================
  Files         196      196              
  Lines       20172    20214      +42     
==========================================
+ Hits        19597    19641      +44     
+ Misses        575      573       -2     
Flag Coverage Δ
unittests 97.16% <100.00%> (+0.01%) :arrow_up:

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

Impacted Files Coverage Δ
autode/units.py 100.00% <ø> (ø)
autode/hessians.py 99.17% <100.00%> (ø)
autode/values.py 100.00% <100.00%> (+0.35%) :arrow_up:
autode/wrappers/XTB.py 98.53% <100.00%> (-0.03%) :arrow_down:
tests/test_hessian.py 98.59% <100.00%> (+0.02%) :arrow_up:
tests/test_value.py 100.00% <100.00%> (ø)
tests/test_values.py 98.43% <100.00%> (+0.28%) :arrow_up:
tests/test_wrappers/test_xtb.py 99.47% <100.00%> (+0.04%) :arrow_up:
autode/plotting.py 96.66% <0.00%> (+0.83%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

t-young31 commented 1 year ago

@shoubhikraj do you think it's worth instead having to and to_ for copy and inplace like pytorch? This is kinda a breaking change (i.e. should go to v1.4.0) but if you think it's not obvious that an inplace modification might happen then I'm sure others do too!

shoubhikraj commented 1 year ago

@t-young31 I have thought about this a bit, and I think it is worth having to and to_ for copy and inplace edit. But then to_ would not be available for floating point value types (as they are immutable, so inplace modification does not work?). to_ would only work on valuearray types. What do you think?

t-young31 commented 1 year ago

Cool – cheers. I think to_ being only available for arrays I think makes sense. Will add 😄