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

New parameter: config_gvf_update #49

Merged
merged 11 commits into from
Sep 27, 2024

Conversation

tanyasmirnova
Copy link
Collaborator

The new configuration parameter: config_gvf_update is added to core_atmosphere. When config_gvf_update = true, then green vegetation fraction is updated from climatology, when false - the GVF value from *init.nc is used for the length of the forecast.
With config_gvf_update=false, the real-time VIIRS grean vegetation fraction from RAP or RRFS is used in the model.

Enter a description of this PR. This should include why this PR was created, and what it does.
With config_gvf_update=false, the real-time VIIRS green vegetation fraction from RAP or RRFS is used in the MPAS model.

Testing and relations to other Pull Requests should be added as subsequent comments.

See the below examples for more information.
MPAS-Dev/MPAS#930
MPAS-Dev/MPAS#931

then green vegetation fraction is updated from climatology, when false - the GVF value
from *init.nc is used for the length of the forecast.
@tanyasmirnova
Copy link
Collaborator Author

@jderrico-noaa Janet, could you lease merge this really small PR. It is a work around the overwriting vegetation green fraction with climatology. The default for the new config parameter is .true., thus the NCAR MPAS will run as is, but in our MPAS_GSL we have to add a nameist option: config_gvf_update=.false. Thank you!

@barlage
Copy link
Collaborator

barlage commented Aug 23, 2024

@tanyasmirnova can you run a test without your changes and

config_sfc_albedo = .false.

if your namelist is setting this to true. I don't know what the default is.

There seems to be some conflation in the code between greeness (or greenness) updates and albedo updates. The alarm frequency is set with greenness but the actually update is triggered by albedo (I believe).

Alternatively, I can think of a one line change that should also solve what you are trying to do without adding anything to the namelist.

@tanyasmirnova
Copy link
Collaborator Author

@barlage Mike, config_sfc_albedo is used only inphysics/mpas_atmphys_update_surface.F to interpolate surface albedo from the climatology to the current day:!updates the surface background albedo for the current date as a function of the monthly-mean
!surface background albedo valid on the 15th day of the month, if config_sfc_albedo is true:
if(config_sfc_albedo) then

call monthly_interp_to_date(nCellsSolve,current_date,albedo12m,sfc_albbck)

do iCell = 1, nCellsSolve
   sfc_albbck(iCell) = sfc_albbck(iCell) / 100.
   if(landmask(iCell) .eq. 0) sfc_albbck(iCell) = 0.08
enddo

endif

So it does not control greenness inside mpas_atmphys_update_surface.F. My change does not call this subroutine if config_gvf_update=false. It is not good, because if config_sfc_albedo=true, we have to update albedo independently from GVF. To fix this, I'll pass into mpas_atmphys_update_surface.F config_gvf_upate, and put the 'if' only around the greenness update.
Overall, the updating of surfce variables in MPAS is very messy and inconsistent.

that if config_gvf_update=false, the VEGFRA is not overwritten by the
climatology.
@tanyasmirnova
Copy link
Collaborator Author

I have updated my PR as I explained in the message to @barlage. The test with updated code runs fine.

…ion when nvegopt=2, but there is no

green vegetation fraction in the RAP/RRFS grib output.
Copy link
Collaborator

@barlage barlage left a comment

Choose a reason for hiding this comment

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

My overall issue here is whether all these additions are necessary and will complicate merging in the future. The config_gvf_update flag seems redundant to config_greeness_update = "none". Why not just send config_greeness_update into physics_surface_update and check if it is "none"?

call mpas_set_timeInterval(alarmTimeStep,timeString=config_greeness_update,ierr=ierr)
alarmStartTime = startTime
call mpas_add_clock_alarm(clock,greenAlarmID,alarmStartTime,alarmTimeStep,ierr=ierr)
if (config_gvf_update) then
Copy link
Collaborator

Choose a reason for hiding this comment

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

@tanyasmirnova this if block seems unnecessary now and maybe even not working correctly if you want to update albedo and not vegetation, in that case greenAlarmID needs to be set and ringing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@barlage To begin with, it is a very bad set-up to do albedo and greenness updating connected to each other. Our situation proves it not robust, because we treat greenness in a special way. I would rewrite surface updating but as you said there wil be more issues with merging in future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I chose the least invasive solution to get the real-time greenness vegetation fraction from the parent model to MPAS. I see some problems in MPAS with surface updating, but my understanding that we are not supposed to change any of their code, it is "untouchable" because of future commits. Therefore, I added a new config_gvf_update parameter to achieve our goal.

@tanyasmirnova this if block seems unnecessary now and maybe even not working correctly if you want to update albedo and not vegetation, in that case greenAlarmID needs to be set and ringing

I have removed the if(config_gvf_update) from line 554. So - the "green" alarm will be still created, but I pass config_gvf_update into physics_update_surface, and when it is set to false, the GVF will not be overwritten with climatology, but background albedo will be created. At initial forecast time surface_update repeats what was already done in mpas_atmphys_initialize_real.F at the INIT step. Therefore, background albedo was already computed. And we do not want update the background albedo in our MPAS runs. The actual albedo will be updated for grid points covered with snow. In RRFS we also update it for low solar angles, but so far not in MPAS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@barlage Mike, could you please revisit my PR. I think I took into account all your comments and the PR should be ready to merge. Thank you.

@tanyasmirnova
Copy link
Collaborator Author

My overall issue here is whether all these additions are necessary and will complicate merging in the future. The config_gvf_update flag seems redundant to config_greeness_update = "none". Why not just send config_greeness_update into physics_surface_update and check if it is "none"?

My overall issue here is whether all these additions are necessary and will complicate merging in the future. The config_gvf_update flag seems redundant to config_greeness_update = "none". Why not just send config_greeness_update into physics_surface_update and check if it is "none"?

The problem with config_greeness_update is that it is called misleading. It is actually the interval of updating and if you set it to 'none' the model crashes.

modified:   core_init_atmosphere/mpas_init_atm_cases.F
Returned nveg_opt which I erroneously deleted.
Jensen): shdmin and shdmax are in the first guess entity now, not in
mesh.
@jderrico-noaa jderrico-noaa mentioned this pull request Sep 3, 2024
Copy link
Collaborator Author

@tanyasmirnova tanyasmirnova 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 resolved all comments and conflicts.

src/core_atmosphere/physics/mpas_atmphys_manager.F Outdated Show resolved Hide resolved
src/core_atmosphere/physics/mpas_atmphys_manager.F Outdated Show resolved Hide resolved
@barlage
Copy link
Collaborator

barlage commented Sep 5, 2024

@tanyasmirnova check out this suggestion. This will prevent the updating of the VEGFRA that is read from the init file and doesn't require any new configuration option. My understanding here is that by not setting greenAlarmID, the alarm will never ring, even at the beginning of the simulation. I've tested this with a 25 hour simulation.

@tanyasmirnova
Copy link
Collaborator Author

@tanyasmirnova check out this suggestion. This will prevent the updating of the VEGFRA that is read from the init file and doesn't require any new configuration option. My understanding here is that by not setting greenAlarmID, the alarm will never ring, even at the beginning of the simulation. I've tested this with a 25 hour simulation.

@barlage

@tanyasmirnova check out this suggestion. This will prevent the updating of the VEGFRA that is read from the init file and doesn't require any new configuration option. My understanding here is that by not setting greenAlarmID, the alarm will never ring, even at the beginning of the simulation. I've tested this with a 25 hour simulation.

@barlage I have commented on your PR#52. Additional IF is not needed. And in the code I propose, greenAlarmID so it does ring to update albedo, but VEGFRA will not be changed with when config_gvf_update == false. This was the easist way to merge my changes into MPAS without changing any of there code (which is not optimal).

@barlage
Copy link
Collaborator

barlage commented Sep 5, 2024

the model will not crash with this change; as noted I have tested it and it works. The model crashes because of the call inside this if block.

@barlage
Copy link
Collaborator

barlage commented Sep 5, 2024

Note that this is also the same approach that is used for other config options that have "none" as an option, for example config_pbl_interval, so I think is a viable suggestion to propose back to NCAR

@jderrico-noaa jderrico-noaa merged commit b0ac332 into NOAA-GSL:gsl/develop Sep 27, 2024
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.

5 participants