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

merge mixed mode updates into main #1366

Merged
merged 58 commits into from
Sep 20, 2023
Merged

Conversation

rem1776
Copy link
Contributor

@rem1776 rem1776 commented Sep 14, 2023

Description
Merges mixed mode changes from the last tag into main.

How Has This Been Tested?
alpha2

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • make distcheck passes

rem1776 and others added 30 commits November 23, 2022 15:03
Co-authored-by: mlee03 <Mikyung.Lee@lscamd50-d.gfdl.noaa.gov>
@@ -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()
Copy link
Contributor

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

Copy link
Member

@thomas-robinson thomas-robinson left a 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.

astronomy/astronomy.F90 Outdated Show resolved Hide resolved
astronomy/astronomy.F90 Outdated Show resolved Hide resolved
astronomy/astronomy.F90 Outdated Show resolved Hide resolved
astronomy/astronomy.F90 Show resolved Hide resolved
block_control/block_control.F90 Show resolved Hide resolved
coupler/coupler_types.F90 Show resolved Hide resolved
coupler/coupler_types.F90 Show resolved Hide resolved
coupler/coupler_types.F90 Outdated Show resolved Hide resolved
coupler/coupler_types.F90 Outdated Show resolved Hide resolved
coupler/coupler_types.F90 Show resolved Hide resolved
@rem1776
Copy link
Contributor Author

rem1776 commented Sep 18, 2023

@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?

@thomas-robinson
Copy link
Member

@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.

Copy link
Contributor

@bensonr bensonr left a 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).

astronomy/include/astronomy_r4.fh Outdated Show resolved Hide resolved
astronomy/include/astronomy_r8.fh Outdated Show resolved Hide resolved
Comment on lines +37 to +45
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
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@J-Lentz might have more of an answer when he's in, but if we're changing the arguments I think those updates would be done separate from this. I see what you are saying though, I don't see it even using the data domain after its retrieved.

I made an issue in the meantime, #1371

Copy link
Contributor

Choose a reason for hiding this comment

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

@bensonr Should the change be made as part of this PR, or should I address #1371 separately after this PR is merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

@J-Lentz - with the new issue from @rem1776, this can be addressed separately

libFMS.F90 Show resolved Hide resolved
monin_obukhov/include/monin_obukhov_inter_r4.fh Outdated Show resolved Hide resolved
time_interp/include/time_interp_external2_r4.fh Outdated Show resolved Hide resolved
time_interp/time_interp_external2.F90 Show resolved Hide resolved
time_manager/get_cal_time.F90 Outdated Show resolved Hide resolved
time_manager/time_manager.F90 Outdated Show resolved Hide resolved
topography/gaussian_topog.F90 Outdated Show resolved Hide resolved
add rm diag_integral.out to test script
@rem1776
Copy link
Contributor Author

rem1776 commented Sep 19, 2023

@bensonr @thomas-robinson This should be updated for all review comments now, let me know if there's anything I still need to address.

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)
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@bensonr bensonr left a 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...

time_manager/get_cal_time.F90 Show resolved Hide resolved
test_fms/diag_integral/test_diag_integral2.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@bensonr bensonr left a 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.

@rem1776 rem1776 merged commit b25a7c5 into NOAA-GFDL:main Sep 20, 2023
17 of 19 checks passed
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.

6 participants