-
Notifications
You must be signed in to change notification settings - Fork 137
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
merge mixed mode updates into main #1366
Conversation
@@ -492,10 +499,11 @@ module field_manager_mod | |||
integer :: max_index | |||
type (field_def), pointer :: first_field => NULL() | |||
type (field_def), pointer :: last_field => NULL() | |||
integer, pointer, dimension(:) :: i_value => NULL() |
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.
there's a pr open in main to change these to allocatables
https://github.com/NOAA-GFDL/FMS/pull/1211/files
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.
There are a lot of unnecessary changes related to spacing, but I don't think that they all follow the 2 spaces listed in the style guide.
When you change spacing to make it "look nice", you become the owner of that line of code.
@thomas-robinson Review comments should be addressed now, updated the error messaging to provide more info. The only thing i didn't change was initializing the pointers to null. I don't see how that would affect anything, unless its the implied save? |
Yes the implied save, BUT we shouldn't be changing behavior. I also didn't get through all of the error messages. I'm sure there's more that need more detail, but we can work on that in the future. |
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 have looked over about 85% of the files. There were some that were just too long because of create .inc files from the original .F90s.
In astronomy.F90, there are interface variable doxygen comments with wrong information (line 623) or don't have any doxygen documentation (line 646).
interface get_grid_version_1 | ||
module procedure get_grid_version_1_r4 | ||
module procedure get_grid_version_1_r8 | ||
end interface get_grid_version_1 | ||
|
||
interface get_grid_version_2 | ||
module procedure get_grid_version_2_r4 | ||
module procedure get_grid_version_2_r8 | ||
end interface get_grid_version_2 |
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 these functions are used only locally with data_override, we should clean up the interfaces. Why do we pass in the compute domain indices as arguments, yet query the domain for the data and global indices? Unless I am missing something, we can also query the domain for the compute indices.
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.
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.
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.
add rm diag_integral.out to test script
@bensonr @thomas-robinson This should be updated for all review comments now, let me know if there's anything I still need to address. |
time_manager/get_cal_time.F90
Outdated
increment_years = floor(time_increment/12) | ||
increment_months = floor(time_increment) - 12*increment_years | ||
call get_date(base_time, year,month,day,hour,minute,second) | ||
base_time = set_date(year+increment_years,month+increment_months ,day,hour,minute,second) | ||
dt = 86400*days_in_month(base_time) * month_fraction | ||
dt = real( 86400*days_in_month(base_time), r8_kind) * month_fraction | ||
increment_days = floor(dt/86400) |
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.
one more 86400._r8_kind
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.
that line should be fixed, maybe the comparison glitched out:
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.
Just two minor points yet...
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. Thanks to the mixed-precision team for working through the review issues.
Description
Merges mixed mode changes from the last tag into main.
How Has This Been Tested?
alpha2
Checklist:
make distcheck
passes