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

several tweaks, mainly saving method results for re-use, to improve #1752

Merged
merged 3 commits into from Feb 17, 2016

Conversation

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jan 4, 2016

performance of climatology etc. on Rhea or other machines where I/O
is expensive.

WORK IN PROGRESS DO NOT MERGE YET

performance of climatology etc. on Rhea or other machines where I/O
is expensive.
@doutriaux1 doutriaux1 added this to the 2.4.1 milestone Jan 4, 2016
# For isTime(), keep track of whether each id is for a time axis or not, for better performance.
# This dictionary is a class variable (not a member of any particular instance).
idtaxis = {} # id:type where type is 'T' for time, 'O' for other

Copy link
Contributor Author

@doutriaux1 doutriaux1 Jan 4, 2016

@painter1 what happens if you have mutliple files with same axis id, but only some are actually time? Wouldn't this break? I'm thinking auto generated arrays axis_0 etc...

Copy link
Contributor

@painter1 painter1 Jan 4, 2016

Yes, it would break! I've never seen such a thing, but among the changes I've made, this is the one which I was least sure about.

Copy link
Contributor Author

@doutriaux1 doutriaux1 Jan 4, 2016

maybe we should add a test? See if it's already in and if it is and type changes, throw a warning/error? I don't think it will ever happen but of course now that I said it it is bound to happen...

Copy link
Contributor

@painter1 painter1 Jan 4, 2016

The point of this is to not have to figure out what the type is - that involves reading from a file. So the way I would fix it is to use self.idtaxis only if the user does something to enable this feature, e.g. calls a method to initialize it.

we are now caching this after first use
@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Feb 3, 2016

I will have a look at it for review.

@painter1
Copy link
Contributor

@painter1 painter1 commented Feb 17, 2016

Is this good enough to be merged? We need this for release 2.4.1, and the code freeze is tomorrow.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Feb 17, 2016

@painter1 it looks good to me, certain code I couldn't reasonably test since its very specific but overall its good.

+2

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Feb 17, 2016

Our Mac bots are hosed but I am okay if we merge it as it is.

doutriaux1 added a commit that referenced this issue Feb 17, 2016
several tweaks, mainly saving method results for re-use, to improve
@doutriaux1 doutriaux1 merged commit 212f877 into master Feb 17, 2016
1 of 5 checks passed
@doutriaux1 doutriaux1 deleted the climofast branch Feb 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants