Skip to content
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

Remove multi-run logic from calc #178

Merged
merged 4 commits into from
May 4, 2017
Merged

Remove multi-run logic from calc #178

merged 4 commits into from
May 4, 2017

Conversation

spencerahill
Copy link
Owner

Closes #117

Of course we do eventually want to re-implement this, but good in the meantime to get rid of this really ugly and confusing code.

Copy link
Collaborator

@spencerkclark spencerkclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this cleanup!

I think the test failures may be related to dask/distributed#1056

aospy/calc.py Outdated
raise ValueError("`dtype_in_vert` must be either 'pressure' or "
"'sigma' for pressure data")

def _get_input_data(self, var, start_date, end_date, n):
def _get_input_data(self, var, start_date, end_date):
"""Get the data for a single variable over the desired date range."""
logging.info(self._print_verbose("Getting input data:", var))
# If only 1 run, use it to load all data. Otherwise assume that num
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment still relevant?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, will go through all the comments looking for anything else along these lines.

@spencerahill
Copy link
Owner Author

spencerahill commented May 4, 2017

Fixed that comment and snuck in some other minor cleanup in calc.py

I think the test failures may be related to dask/distributed#1056

@spencerkclark I think you're right. Funny that it's not happening for 3.4. I'm comfortable merging as is despite those failures; let me know if you agree. Tests are all passing on my local machine.

Copy link
Collaborator

@spencerkclark spencerkclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me pending the minor fix to the what's new page. I agree that the test failures are unrelated; I say go ahead and merge after filling in the issue numbers.

@@ -30,6 +30,10 @@ Dependencies
Bug Fixes
~~~~~~~~~

- Remove faulty logic for calculations with data coming from multiple
runs. Eventually this feature will be properly implemented (fixes
:issue:`###` via :pull:`17#`). By `Spencer Hill
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fill in issue numbers here (#117 and #178)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! another good catch

@spencerahill
Copy link
Owner Author

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants