-
Notifications
You must be signed in to change notification settings - Fork 168
Refactors and bugfixes #1736
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
Refactors and bugfixes #1736
Conversation
|
Wanting to wrap some more refactorings into here |
4e66000 to
17ed9bd
Compare
|
I didn't fix the #1738 into this. I wanted to do a bit of a deeper refactoring of |
| [os.remove(s) for s in [lib_file] if os.path is not None and os.path.exists(s)] | ||
| if delete_cfiles and len(all_files_array) > 0: | ||
| [os.remove(s) for s in all_files_array if os.path is not None and os.path.exists(s)] | ||
| os.remove(lib_file) | ||
| if delete_cfiles: | ||
| for s in all_files_array: | ||
| if os.path.exists(s): | ||
| os.remove(s) |
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 don't we need to check for os.path is not None here anymore? Was that old, redundant code?
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.
os.path is a module of the os package, so it is never None.
A bit ago we had
from os import path
...
[... if path is not None ...]so it wasn't imediately evident before
parcels/kernel.py
Outdated
| if not g._load_chunk.flags["C_CONTIGUOUS"]: | ||
| g._load_chunk = np.array(g._load_chunk, order="C") | ||
| g._load_chunk = np.array(g._load_chunk, order="C") |
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 is the if-statement not necessary anymore, here?
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.
That was a mistake. Was doing some experimenting which slipped through
parcels/tools/_helpers.py
Outdated
| obj.__doc__ = f"{obj.__doc__ or ''}{extra}".strip() | ||
|
|
||
|
|
||
| @np.vectorize |
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.
Is it really necessary to vectorise this function? Is it really so compute-intense?
Or is this a start of more of these decorators?
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.
Good catch. This decorator is an easy way to make functions numpy compatible, so that you can give it an array and it can apply to each element.
I was initially using this function in TimeConverter.reltime, however since removed it from this PR. Will remove this decorator for now until it is actually needed.
Re. performance, I don't think theres any drawbacks
b351559 to
69b1094
Compare
for more information, see https://pre-commit.ci
timedelta_to_floathelper