-
Notifications
You must be signed in to change notification settings - Fork 52
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
Type checking #277
Type checking #277
Conversation
…ing (duartegroup#202) * make autode compatible with windows: replace multiprocessing with joblib and loky * remove comment autode/utils.py * check total memory in utils * use loky for parallel processing * loky context is used for better handling of parallelisation * fixes for timeout wrapper in POSIX * use class instances instead of class attributes for Config * pass parent Config state into child processes * keep default timeout wrapper, also add new wrapper * separate parallelisation scheme for windows, restore default for linux/mac * fix test_config_in_worker_proc for posix * codecov on windows * add tests for config copy * change config implementation for updating in worker process * add context manager for temporary config, minor fix in experimental timeout for windows * add tests for context manager * use bash for installing xtb in CI runner * add tests for cleanup after timeout * exclude setup.py from codecov * make config picklable and easily readable * add test and minor fixes * move xtb install script to .github/scripts * use temporary_config for test_utils.py * update changelog and contributor list * resolve conflicts with v1.4.0 Co-authored-by: Tom Young <39765193+t-young31@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some comments, though it is a huge amount of code changes, so I need some time to go through all of it. 😄
@@ -198,7 +202,7 @@ def _step(self) -> None: | |||
self._coords = self._coords + step | |||
|
|||
step_size = np.linalg.norm( | |||
self._coords.to("cart") - self._history.penultimate.to("cart") | |||
self._coords.to("cart") - self._history.penultimate.to("cart") # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this cause problems with subtraction of valuearray types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah – to
doesn't return a specific enough type 😕
If I were writing it again I think explicit to_cartesian
methods etc. would be better
@@ -40,7 +40,7 @@ jobs: | |||
|
|||
- name: Install | |||
run: | | |||
conda install \ | |||
mamba install \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why switch from conda to mamba sorry, I don't understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to make the dependency resolution faster. install was taking 20 minutes, and I'm impatient!
https://github.com/duartegroup/autodE/actions/runs/5432666711/jobs/9879860668
(would be awesome if we could get rid of some of them..)
Resolves #237
TODO
Checklist