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

Update MPAS components for v1 ACME #412

Merged

Conversation

douglasjacobsen
Copy link
Member

This merge updates the MPAS components to include the scientific features for ACME v1 simulations.

Additionally, it makes driver changes required to handle frazil ice and BGC between MPAS-O and MPAS-CICE.

[CC]
[NML]

@douglasjacobsen douglasjacobsen self-assigned this Oct 30, 2015
@douglasjacobsen douglasjacobsen force-pushed the douglasjacobsen/mpas/update-mpas-components-v1 branch 2 times, most recently from 751cd1e to f819da9 Compare November 3, 2015 00:18
@douglasjacobsen douglasjacobsen modified the milestone: v1.0 Alpha Nov 3, 2015
@rljacob
Copy link
Member

rljacob commented Nov 3, 2015

BFB? CC?

@worleyph
Copy link
Contributor

On Titan using PGI, the preprocessor (.F to .f90) interprets '' as the last nonblank character on a line as a continuation character. In consequence, the comment lines (1132-1136) in

   mpas_cice_advection_incremental_remap.F 

are turned into one very long comment line, and compilation then fails with the error message:

  PGF90-S-0285-Source line too long (mpas_cice_advection_incremental_remap.f90: 1134)

I replaced these with a different character (not as aesthetically pleasing) and mpas-cice compiled with no problems.

@douglasjacobsen
Copy link
Member Author

@worleyph Thanks for testing this, I'll make sure those changes happen on the MPAS side.

@worleyph
Copy link
Contributor

Also, on Titan and with PGI something is not working right with some of the timer names in mpas-cice. They are 512 characters long, and mostly blank. The list follows (number of characters, nonblank characters):

   512  XXX_conservationCheck
   512  XXX_geographicalVectors
   512  XXX_regionalStatistics
   512  XXX_ridgingDiagnostics
   512  XXX_temperatures

where XXX is one of

   compute
   finalize
   init
   precompute
   reset_alarm
   restart
   write

They all appear to be generated in routines in mpas_cice_analysis_driver.F by logic of the form

   timerName = trim(computeTimerPrefix) // poolItr % memberName(1:nameLength)    

The fix may be to add the following line:

   timerName = trim(timerName)

after each line of the indicated form. I'll update after my test job runs, but I may not get to this until tomorrow. ... Nope - that did not change anything. Initializing the variable timerName to " " did nothing either. I'll keep looking for a fix, but if someone know how to fix this, please speak up :-).

@douglasjacobsen
Copy link
Member Author

@worleyph
Copy link
Contributor

@douglasjacobsen , yes, those lines.

@whlipscomb
Copy link

@douglasjacobsen and @worleyph, Could we fix the offending lines by putting exclamation points at the end, like so?

   !                            /      E0       \                                      !                                      

@douglasjacobsen
Copy link
Member Author

Changing the \ characters to | works. I'm testing adding an ending ! as @whlipscomb suggested now.

@worleyph
Copy link
Contributor

Update on long mpas-cice timers. The problem is not in the string operations in mpas_cice_analysis_driver.F . The string sizes in mpas_cice_analysis_driver.F are accurate. The string sizes are not accurate inside of mpas_timer.F . The logic is

      trimed_name = trim(timer_name)
      nlen = len(trimed_name)

   ...

   #ifdef MPAS_PERF_MOD_TIMERS
      call t_startf(trimed_name)
   !pw++                       
      if ( domain_info % my_proc_id == 0 ) then
         if (nlen > 50) then
            write(6,*) "Long Timer Name:", nlen, trimed_name
    call flush(6)
         endif
      endif
   !pw-- 

The nlen is "accurate" in that trimed_name is 512 characters long.

@douglasjacobsen
Copy link
Member Author

Adding ! at the end does fix the issue as well. I've submitted a PR for MPAS to have the issue fixed.

@douglasjacobsen
Copy link
Member Author

@worleyph Can you test the following for the long timer names?

NOTE: You have to make the change in all MPAS components that you will be using in a run (i.e. if you are using the B case, you need to make the change in all MAPS-O, MPAS-LI, and MPAS-CICE).

Change this line:
https://github.com/ACME-Climate/MPAS/blob/cice/develop/src/framework/mpas_timer.F#L75

From: character (len=len(timer_name)) :: trimed_name
To: character (len=len_trim(timer_name)) :: trimed_name

And this line:
https://github.com/ACME-Climate/MPAS/blob/cice/develop/src/framework/mpas_timer.F#L230

From: character (len=len(timer_name)) :: trimed_name !< Trimed timer name
To: character (len=len_trim(timer_name)) :: trimed_name !< Trimed timer name

@worleyph
Copy link
Contributor

@douglasjacobsen , will do. I'm trying passing in trim(timer_name) directly to t_startf/t_stopf at the moment. Once this is complete (maybe 30 minutes), I'll try your fix.

@worleyph
Copy link
Contributor

Okay, replacing

      trimed_name = trim(timer_name)
      call t_startf(trimed_name)

by

      call t_startf(trim(timer_name))

(and similarly for t_stopf) worked. I'll try the above next.

@worleyph
Copy link
Contributor

@douglasjacobsen , this change

   From: character (len=len(timer_name)) :: trimed_name
   To: character (len=len_trim(timer_name)) :: trimed_name

also worked. I'll start using this.

Thanks.

Pat

jonbob and others added 2 commits December 3, 2015 14:07
This commit updates the driver_cpl code to include BGC and frazil ice
formation fields, which will be needed by the ocean and sea ice
components.

[NML]
This commit updates the versions of the MPAS components to their v1
versions.

Specifically, versions are:

mpas-cice: da0748c
mpas-o: a1bc4e5
mpasli: 216448c

Changes included are:

MPAS-CICE:
    - Advection
    - BGC
    - Improved column package
    - Improved timers
    - Analysis infrastructure

MPAS-O:
    - Several bug fixes
    - Additional analysis members
    - OpenMP
    - Performance improvements
    - Improved timers

MPAS-LI:
    - Calving law
    - Thermodynamic solver
    - Analysis infrastructure
@douglasjacobsen douglasjacobsen force-pushed the douglasjacobsen/mpas/update-mpas-components-v1 branch from 1b99456 to aac0736 Compare December 8, 2015 22:48
douglasjacobsen added a commit that referenced this pull request Dec 8, 2015
…xt (PR #412)

This merge updates the MPAS components to include the scientific
features for ACME v1 simulations.

Additionally, it makes driver changes required to handle frazil ice
between MPAS-O and MPAS-CICE, and to add BGC fields to MPAS-CICE.

[CC]
[NML]

* douglasjacobsen/mpas/update-mpas-components-v1:
  Update MPAS components for ACME v1 simulations
  Modifying driver_cpl code to include BGC and frazil fields
@douglasjacobsen
Copy link
Member Author

Merged to next

@douglasjacobsen douglasjacobsen merged commit aac0736 into master Dec 9, 2015
douglasjacobsen added a commit that referenced this pull request Dec 9, 2015
This merge updates the MPAS components to include the scientific
features for ACME v1 simulations.

Additionally, it makes driver changes required to handle frazil ice and
BGC between MPAS-O and MPAS-CICE.

[CC]
[NML]

* douglasjacobsen/mpas/update-mpas-components-v1:
  Update MPAS components for ACME v1 simulations
  Modifying driver_cpl code to include BGC and frazil fields
@douglasjacobsen douglasjacobsen deleted the douglasjacobsen/mpas/update-mpas-components-v1 branch December 9, 2015 21:34
jgfouca pushed a commit that referenced this pull request Feb 27, 2018
This merge updates the MPAS components to include the scientific
features for ACME v1 simulations.

Additionally, it makes driver changes required to handle frazil ice and
BGC between MPAS-O and MPAS-CICE.

[CC]
[NML]

* douglasjacobsen/mpas/update-mpas-components-v1:
  Update MPAS components for ACME v1 simulations
  Modifying driver_cpl code to include BGC and frazil fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants