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

Don't construct lat/long arrays before scattering. #59

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

groutr
Copy link

@groutr groutr commented Nov 20, 2020

This should reduce the overhead of scattering the arrays. Using broadcasting also reduces memory usage.

@rcabell rcabell self-requested a review November 20, 2020 17:20
@rcabell
Copy link
Collaborator

rcabell commented Nov 20, 2020

Thanks for these optimizations! I'll review them soon and do timing/regression tests. I have some other changes that need to go in ASAP, so this may be a few days, and need to be rebased, but I'll take care of that.

@groutr
Copy link
Author

groutr commented Nov 20, 2020

Should the assumption be made that latitude and longitude have the same dimensions?

@rcabell
Copy link
Collaborator

rcabell commented Nov 20, 2020

Should the assumption be made that latitude and longitude have the same dimensions?

No, we can't assume that. For example, in GFS, the latitude dimension is 1536 and the longitude is 3072.

@groutr
Copy link
Author

groutr commented Nov 20, 2020

by dimensions, I meant number of dimensions. sorry.

@rcabell
Copy link
Collaborator

rcabell commented Nov 20, 2020

by dimensions, I meant number of dimensions. sorry.

I still think that assumption isn't safe; in some input files the latitude and longitude fields are 2D, and in some there is no time dimension on the climate fields.

@groutr
Copy link
Author

groutr commented Nov 20, 2020

by dimensions, I meant number of dimensions. sorry.

I still think that assumption isn't safe; in some input files the latitude and longitude fields are 2D, and in some there is no time dimension on the climate fields.

The reason I asked is because that assumption was being made implicitly in the code. I added some checks that hopefully can catch odd inputs.

@groutr
Copy link
Author

groutr commented Nov 23, 2020

@rcabell are the regression tests publicly available?

@rcabell
Copy link
Collaborator

rcabell commented Nov 30, 2020

@rcabell are the regression tests publicly available?

No, the tests are currently a bit ad-hoc and not yet available publicly.

@rcabell rcabell added the future development Under consideration for future development label Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future development Under consideration for future development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants