-
Notifications
You must be signed in to change notification settings - Fork 13
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
Recursively compute variables #263
Recursively compute variables #263
Conversation
I couldn't find a |
Note of course this doesn't deal with saving intermediates to disk (or checking for saved intermediates), which is also discussed in #3, but I think what's here is already a reasonable improvement. |
Wow, this is surprisingly elegant! The logic and tests look good to me. One big thing: have we tested this out within a Besides that, a couple documentation requests:
|
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 @spencerahill! I agree this should be highlighted in the documentation. It will certainly simplify my object library quite a bit. I'll add a full docstring to recursively_compute_variable
and an example to the official docs sometime later today or tomorrow.
if isinstance(files_list, str): | ||
files_list = [files_list] | ||
subprocess.call(['dmget'] + files_list) | ||
subprocess.call(['dmget'] + archive_files) |
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 was finding that the test suite would hang if dmget
was called on an existing file that was not stored in /archive
. I was able to reproduce this outside of aospy, so it has nothing to do with the changes I made to allow for the recursive calculation of variables; they must have made some updates to dmget
. The logic here filters out files that aren't stored on archive before passing them to dmget
.
aospy/data_loader.py
Outdated
@@ -296,7 +296,7 @@ def recursively_compute_variable(self, var, start_date=None, end_date=None, | |||
data = [self.recursively_compute_variable( | |||
v, start_date, end_date, time_offset, **DataAttrs) | |||
for v in var.variables] | |||
return var.func(*data) | |||
return var.func(*data).rename(var.name) |
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.
Indeed, good suggestion to add a test for this. One minor change and things work smoothly!
All sounds/looks good. Just ping me when you're ready for final review. |
- Add a full docstring for compute_variable_recursively - Add an example of this functionality to the official documentation - Make test more succinct in test_calc_basic
I guess I spoke too soon in #259; that's the failing test here :) @spencerahill anyway, whenever you get a chance, this should be ready for a final review! |
Bummer. Still, it seems like the frequency of the failures has gone down...maybe it's on its way toward going away completely. In it goes! Thanks @spencerkclark, this is great. |
Closes #3
This allows one to compose variable objects whose functions take other computed variables as arguments. For example: