Skip to content

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

Merged
doutriaux1 merged 3 commits intomasterfrom
climofast
Feb 17, 2016
Merged

several tweaks, mainly saving method results for re-use, to improve#1752
doutriaux1 merged 3 commits intomasterfrom
climofast

Conversation

@doutriaux1
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
Copy Markdown
Contributor

I will have a look at it for review.

@painter1
Copy link
Copy Markdown
Contributor

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

@aashish24
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

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

doutriaux1 added a commit that referenced this pull request Feb 17, 2016
several tweaks, mainly saving method results for re-use, to improve
@doutriaux1 doutriaux1 merged commit 212f877 into master Feb 17, 2016
@doutriaux1 doutriaux1 deleted the climofast branch February 17, 2016 18:53
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.

4 participants