-
Notifications
You must be signed in to change notification settings - Fork 14
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
fix real32 gnu issue #340
base: development
Are you sure you want to change the base?
fix real32 gnu issue #340
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.
Thanks for the bug fixes! I had a few optional requests but they certainly aren't necessary if they start to hold up this PR.
@@ -1533,11 +1537,21 @@ subroutine config_write_field(this, field, split_file_index, restart, & | |||
end_dims = field%end_dims() | |||
frank = size(field_shape) | |||
if (frank == 1) then | |||
allocate(field_data(end_dims(1) - beg_dims(1) + 1, 1), stat=ierr) | |||
call check_allocate(ierr, subname, 'field_data', file=__FILE__, line=__LINE__-1) | |||
if (trim(field_precision) == 'REAL32') then |
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.
Didn't we recently learn that you don't need to trim
the string variable in order to do the string comparison? If so then I would vote to remove the trim
calls here and below.
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.
I think that's only true for comparison vs an array of strings (e.g. "any" syntax) where you don't have to trim each array element. Like:
if (any(freq_types_to_check == trim(this%output_freq_type))) then
but i may be misremembering!
src/history/cam_hist_file.F90
Outdated
call check_allocate(ierr, subname, 'field_data', file=__FILE__, line=__LINE__-1) | ||
if (trim(field_precision) == 'REAL32') then | ||
allocate(field_data_r4(end_dims(1) - beg_dims(1) + 1, 1), stat=ierr) | ||
call check_allocate(ierr, subname, 'field_data_r4', file=__FILE__, line=__LINE__-1) |
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.
Now that check_allocate
supports passing in errmsg
as well can we do that here and below? Thanks!
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.
added!
src/history/cam_hist_file.F90
Outdated
call cam_grid_write_dist_array(this%hist_files(split_file_index), field_decomp, dim_sizes(1: frank), & | ||
field_shape, field_data_r4(:,1), varid) | ||
else | ||
call hist_buffer_norm_value(buff_ptr, field_data_r8(:,1)) | ||
call cam_grid_write_dist_array(this%hist_files(split_file_index), field_decomp, dim_sizes(1: frank), & |
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.
If we know frank
equals one then I think we can simplify this slightly to the following:
call cam_grid_write_dist_array(this%hist_files(split_file_index), field_decomp, dim_sizes(1: frank), & | |
field_shape, field_data_r4(:,1), varid) | |
else | |
call hist_buffer_norm_value(buff_ptr, field_data_r8(:,1)) | |
call cam_grid_write_dist_array(this%hist_files(split_file_index), field_decomp, dim_sizes(1: frank), & | |
call cam_grid_write_dist_array(this%hist_files(split_file_index), field_decomp, dim_sizes(1), & | |
field_shape, field_data_r4(:,1), varid) | |
else | |
call hist_buffer_norm_value(buff_ptr, field_data_r8(:,1)) | |
call cam_grid_write_dist_array(this%hist_files(split_file_index), field_decomp, dim_sizes(1), & |
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.
cam_grid_write_dist_array
is expecting an array, so updated to
field_decomp, (/dim_sizes(1)/),
src/history/cam_hist_file.F90
Outdated
call cam_grid_write_dist_array(this%hist_files(split_file_index), field_decomp, dim_sizes(1: frank), & | ||
field_shape, field_data_r4, varid) | ||
else | ||
call hist_buffer_norm_value(buff_ptr, field_data_r8) | ||
call cam_grid_write_dist_array(this%hist_files(split_file_index), field_decomp, dim_sizes(1: frank), & |
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.
Very silly nit-pick but can we remove the space between 1:
and frank
here?
call cam_grid_write_dist_array(this%hist_files(split_file_index), field_decomp, dim_sizes(1: frank), & | |
field_shape, field_data_r4, varid) | |
else | |
call hist_buffer_norm_value(buff_ptr, field_data_r8) | |
call cam_grid_write_dist_array(this%hist_files(split_file_index), field_decomp, dim_sizes(1: frank), & | |
call cam_grid_write_dist_array(this%hist_files(split_file_index), field_decomp, dim_sizes(1:frank), & | |
field_shape, field_data_r4, varid) | |
else | |
call hist_buffer_norm_value(buff_ptr, field_data_r8) | |
call cam_grid_write_dist_array(this%hist_files(split_file_index), field_decomp, dim_sizes(1:frank), & |
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.
the picked nit has been squashed (?)
...updated
Tag name (required for release branches): sima0_0_003
Originator(s): peverwhee
Description (include the issue title, and the keyword ['closes', 'fixes', 'resolves'] followed by the issue number):
Select correct field_data buffer (r8 vs r4) depending on the precision of the file in question
closes #336 "History using REAL32 does not appear to write properly"
closes #341 "History code always outputs an "empty" zero timestep"
addresses #343 "Add intel snapshot testing"
Describe any changes made to build system: n/a
Describe any changes made to the namelist: n/a
List any changes to the defaults for the input datasets (e.g. boundary datasets): n/a
List all files eliminated and why: n/a
List all files added and what they do: n/a
List all existing files that have been modified, and describe the changes:
(Helpful git command:
git diff --name-status development...<your_branch_name>
)M src/history/cam_hist_file.F90
M test/existing-test-failures.txt
M cime_config/testdefs/testlist_cam.xml
M cime_config/testdefs/testmods_dirs/cam/outfrq_kessler_derecho/user_nl_cam
If there are new failures (compared to the
test/existing-test-failures.txt
file),have them OK'd by the gatekeeper, note them here, and add them to the file.
If there are baseline differences, include the test and the reason for the
diff. What is the nature of the change? Roundoff?
derecho/intel/aux_sima: All DIFF because of the nstep=0 bug, but confirmed that the results were b4b with a slice of the baselines (removing the erroneous nstep=0 sample)
derecho/gnu/aux_sima: All DIFF because of the nstep=0 bug, but confirmed that the results were b4b with a slice of the baselines (removing the erroneous nstep=0 sample)
If this changes climate describe any run(s) done to evaluate the new
climate in enough detail that it(they) could be reproduced:
CAM-SIMA date used for the baseline comparison tests if different than latest: