-
Notifications
You must be signed in to change notification settings - Fork 4
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 usage of Fortran copy of field data, part 1 #1128
Conversation
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.
nice 👍
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.
🎉 looks good to me!
a couple of unrelated naive questions for my understanding of this code in the comments below.
idx = self%geom%atlas_ij2idx(i,j) | ||
data_m(1, idx) = data_a(1, idx) | ||
do k = 1, tocn_a%shape(1) | ||
data_m(1, idx) = data_m(1, idx) + & |
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.
Double-checking since I don't know this code (and don't feel like thinking, I guess): both in develop and here is it intended to have data_m = data_m + ...
? (I see all other variables updated as data_m = data_a + ...
.
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.
yes, it's different here because 3D variables are combined into a 2D variable. There is an initial data_m
= data_a
on line 299 (2D) then the loop is a summation over 3D data putting into the 2D data_m
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.
... in fact, when I refactored this i did have data_m = data_a +
at first and couldn't figure out why it was giving completely wrong answers !
if (associated(data_cice)) then | ||
data_a(1, idx) = data_a(1, idx) + & | ||
self%kct(i,j) * data_cice(1, idx) | ||
end if |
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.
similar question here: intended to have data_a = data_a + ...
as opposed to data_a = data_m + ...
Description
Part 1 of 3 to address
This refactors
Model2GeoVaLs
andBalance
so that:field%val
structuresync_to_atlas()
andsync_from_atlas()
calls in the interface for these classes were removed, which should make these classes go fasterBalance
, it was wrong, and there were switch statements inside the loops, eeek. Should be faster now2 more PRs after this before we can remove the Fortran-only internal field data and free up some memory!
Testing
There are no change in any answers.
I also tested with the 1/4 deg, 1/2 deg minimization on my computer, 24 cores, there are no changes in answers and runtime for these components is faster (not much, and would be even less with more PEs, but every little bit counts)
develop branch
this branch