-
Notifications
You must be signed in to change notification settings - Fork 113
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
Follow RTD deprecation #563
Conversation
@@ -473,7 +473,7 @@ def set_t(self, t): | |||
Helper function for setting time in-place. | |||
""" | |||
|
|||
self.t.itemset(t) |
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.
This function is meant to change self.t
in place. The new implementation creates a new np.array
for self.t
. I can't recall other usages of self.t
that assume a persistent memory. If that exists, the new implementation will fail: even though t
is being updated here, the previous memory is being referred elsewhere.
Can you check: 1) if this function is called elsewhere? 2) any other implementation that changes t
in place? I know numpy scalars are immutable; that's why itemset
was used.
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.
itemset
is only used in the reset()
method:
Lines 458 to 476 in 5ab784b
def reset(self): | |
""" | |
Reset array sizes to zero and clear all arrays. | |
""" | |
self.set_t(0.0) | |
self.m = 0 | |
self.n = 0 | |
self.o = 0 | |
self.resize_arrays() | |
self.clear_ijv() | |
self.clear_ts() | |
def set_t(self, t): | |
""" | |
Helper function for setting time in-place. | |
""" | |
self.t.itemset(t) |
I changed to in-place assignment, and the test shows below:
print(f"Memory address before: {ss.dae.t.ctypes.data}")
ss.dae.reset()
print(f"Memory address after: {ss.dae.t.ctypes.data}")
Memory address before: 4393601808
Memory address after: 4393601808
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.
Thanks. Looks good! I didn't know triple dots do the trick!
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 learnt this from GitHub Copilot just right now, a positive AGI datapoint!
Follow RTD's deprecation of Sphinx context injection at build time