bgrid_solo scripting and documentation update

[mjs2369]

Description:

• Updates the script workshop_setup.sh to include additional steps that are necessary for it to run successfully: cp perfect_output.nc filter_input.nc and setting
&filter_nml
perturb_from_single_instance to .true. in input.nml
• Updates the documentation for the bgrid_solo model to include a list of steps to build and run filter manually (the same steps executed in workshop_setup.sh). This makes the process more transparent to the user and should also make it easier for our users to know which input files they need and how they are being used.
• Removes the unnecessary file bgrid_solo/work/filter_input.cdl.REMOVED.git-id

Fixes issue

fixes #647

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

Documentation changes needed?

• My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Tests

• Compiled and ran bgrid_solo model, both with and without the use of workshop_setup.sh
• Manually built the documentation

Checklist for merging

• Updated changelog entry
• Documentation updated
• Update conf.py

Checklist for release

• Merge into main
• Create release from the main branch with appropriate tag
• Delete feature-branch

Testing Datasets

• Dataset needed for testing available upon request
• Dataset download instructions included
• No dataset needed

[nancycollins]

i'm sorry if i missed this discussion, but we had originally not put netcdf files into the repo, partly because they're binary and can't be easily diff'd to see differences. instead we distributed the input for perfect model and filter as text .cdl files. the standard netcdf utility 'ncgen' used to be run in the workshop setup script to generate the .nc files at run time. was there a decision that this wasn't a good option?

edit to add: there is still both a perfect_input.cdl as well as a perfect_input.nc in the bgrid_solo dir. i think running filter from a single ensemble member is a smart choice - did the old filter_input.cdl have something like 20 members so it was too big?

[hkershaw-brown]

edit to add: there is still both a perfect_input.cdl as well as a perfect_input.nc in the bgrid_solo dir. i think running filter from a single ensemble member is a smart choice - did the old filter_input.cdl have something like 20 members so it was too big?

Its unclear what the decisions where:

The file in the repo appears to be a SHA (maybe for svn, doesn't seem to match DART or DART_development
models/bgrid_solo/work/filter_input.cdl.REMOVED.git-id

The history is not helpful either https://github.com/NCAR/DART/commits/main/models/bgrid_solo/work/filter_input.cdl.REMOVED.git-id

and why this change was committed as part of 7aac084 - can't tell what the goal was here either.

the workshop_setup.sh{csh} hasn't worked for 7+ years.
Recently someone #647 was trying to use bgrid_solo and was understandably confused what the file filter_input.cdl.REMOVED.git-id was. Time to get rid of it, and fix the workshop_setup script.

[nancycollins]

i think some of the breakage happened when quickbuild was changed (and improved). the original older quickbuild used to look for .cdl files and run ncgen on the to make a .nc file on demand. then the workshop_setup scripts didn't have to do anything and they ran. i assume the new quickbuild doesn't do that anymore. it seems the choices are to remove the .cdl files from the repo (they exist in many of the lower order models) and commit the corresponding .nc files, or see if the quickbuild could generate .nc files on demand if a .cdl file exists.

[hkershaw-brown]

Thanks for tidying up this issue Marlee.

I've added some comments on your new documentation, I think it is worth thinking about what you want people to takeaway from your documentation.

There is documentation in the workshop_setup.sh header that doesn't match the directory or your new documentation.

[hkershaw-brown]

i think some of the breakage happened when quickbuild was changed (and improved). the original older quickbuild used to look for .cdl files and run ncgen on the to make a .nc file on demand. then the workshop_setup scripts didn't have to do anything and they ran. i assume the new quickbuild doesn't do that anymore. it seems the choices are to remove the .cdl files from the repo (they exist in many of the lower order models) and commit the corresponding .nc files, or see if the quickbuild could generate .nc files on demand if a .cdl file exists.

the new quickbuild.sh runs ncgen. The brid_solo workshop script has not had a filter_input.cdl for 7+ years. bgrid_solo workshop_setup.csh did not work pre the new quickbuild.sh

[nancycollins]

never mind any of my previous comments. i was completely confused about things.

[kdraeder]

I accidentally clicked "start review" instead of "add comment".
I don't see a way to cancel the review, so I'm submitting it with "Request Changes", but I'll add more comments.
So far I've focused on the proposed changes, but I have other comments which I'll make when the current changes are resolved.

[kdraeder]

Is workshop_setup.sh in work because it runs quickbuild.sh?
To me it seems more consistent with the directory structure and naming
to put it in shell_scripts and edit it to refer to ../work/... as needed,
and maybe run the OSSE in ../test.

[nancycollins]

i'm not sure about the bgrid_solo model, but workshop_setup.sh was in the work directory for some of the lorenz models because the tutorial used it to build and run pmo and filter so users could run the diagnostics right away. this whole script may have outlived its usefulness.

[kdraeder]

i'm not sure about the bgrid_solo model, but workshop_setup.sh was in the work directory for some of the lorenz models because the tutorial used it to build and run pmo and filter so users could run the diagnostics right away. this whole script may have outlived its usefulness.

Thanks for the context! I can see how it's simpler in the context of a workshop
to do all the steps in one directory, since there's limited output and number of steps.
This could be an opportunity to avoid the common development path of taking
whatever special purpose code exists and generalizing it just enough for the next goal,
while declining to purge "baggage" code if it's not in the way.
Or maybe this is too entrenched by now.

[mjs2369]

i'm not sure about the bgrid_solo model, but workshop_setup.sh was in the work directory for some of the lorenz models because the tutorial used it to build and run pmo and filter so users could run the diagnostics right away. this whole script may have outlived its usefulness.

@hkershaw-brown @nancycollins @kdraeder

Are we thinking we remove workshop_setup.sh all together then and update the docs accordingly? Or should we keep it in the repo?

[mjs2369]

Update: removing the workshop_setup script from the repo and updating the docs accordingly.

[kdraeder]

I pulled the latest version and read through the readme.rst. Here are my top items or questions.
Namelist:

There's no directory INPUT or template {atmos_model,?}.res file.
I see several options;

1. remove the reference to INPUT,
2. note the absence (and that it's "never" used?)
3. add an option to the 'resolution' paragraph about getting resolution from running the model from a cold start, using the resolution variables in the model's namelist, and finding atmos_model.res in directory RESTART.

INPUT/atmos_model.res is hard-wired in fms_src/atmos_solo/atmos_model.f90. I also see
that bgrid_prog_var.res is hard-wired in fms_src/atmos_bgrid/tools/bgrid_prog_var.f90.
Should all of the atmos_bgrid (spectral dynamical core?) be ignored because DART uses only atmos_solo?

Inconsistent formatting for programs and files;
item in the main text
italicized in the template file
no formatting in the other namelist entries

I have not worked through the test steps (an excellent addition!), but I will if that would be helpful.

I also have a readme.rst with some typo fixes and suggested formatting.
After we've answered my questions about model resolution, I could add in text about that
and push my version for review.

[mjs2369]

I have not worked through the test steps (an excellent addition!), but I will if that would be helpful.

I also have a readme.rst with some typo fixes and suggested formatting. After we've answered my questions about model resolution, I could add in text about that and push my version for review.

@kdraeder no need to work through the test steps, I have already done so. I will also update the formatting for programs and files.

As for the model resolution, I think that the docs are okay in the sense that they already take approach number 2. Here it talks about how there is no INPUT directory the majority of the time.

INPUT/atmos_model.res is hard-wired in fms_src/atmos_solo/atmos_model.f90. I also see
that bgrid_prog_var.res is hard-wired in fms_src/atmos_bgrid/tools/bgrid_prog_var.f90.
Should all of the atmos_bgrid (spectral dynamical core?) be ignored because DART uses only atmos_solo?

I'm not sure if what we should do with the atmos_bgrid stuff. It is never used in DART, should we remove it @hkershaw-brown ?

[hkershaw-brown]

I'm not sure if what we should do with the atmos_bgrid stuff. It is never used in DART, should we remove it @hkershaw-brown ?

are we talking about the fms_src directory? If so the bgird is subroutine callable, so this code is used when the model is advanced in filter or perfect_model_obs.

DART/models/bgrid_solo/work/quickbuild.sh

Line 15 in 924182f

EXTRA=extra_source.path_names

This file has all the bgrid src files that are compiled into dart.
https://github.com/NCAR/DART/blob/main/models/bgrid_solo/work/extra_source.path_names

[mjs2369]

I'm not sure if what we should do with the atmos_bgrid stuff. It is never used in DART, should we remove it @hkershaw-brown ?

are we talking about the fms_src directory? If so the bgird is subroutine callable, so this code is used when the model is advanced in filter or perfect_model_obs.

DART/models/bgrid_solo/work/quickbuild.sh

Line 15 in 924182f

EXTRA=extra_source.path_names

This file has all the bgrid src files that are compiled into dart.
https://github.com/NCAR/DART/blob/main/models/bgrid_solo/work/extra_source.path_names

@kdraeder does this answer all your questions? Either way, please go ahead and push your doc fixes or make them suggestions on the PR. Thanks!

[mjs2369]

Thanks for your help @kdraeder @hkershaw-brown !

I think we are ready to release this - if one of you could just take one last quick look through and hopefully approve the PR

[kdraeder]

The only minor flaw that I see is that the updated format of the file names
in the namelist descriptions causes bits of text to be hidden.
It's still easily readable, and I don't know of an easy fix in rst, so it's fine with me to call it done.

[mjs2369]

The only minor flaw that I see is that the updated format of the file names in the namelist descriptions causes bits of text to be hidden. It's still easily readable, and I don't know of an easy fix in rst, so it's fine with me to call it done.

I'm not seeing that in the docs that I've bit manually or in the docs built by the Github actions check. No text is hidden here on my end. Could you provide a screenshot?

[kdraeder]

The only minor flaw that I see is that the updated format of the file names in the namelist descriptions causes bits of text to be hidden. It's still easily readable, and I don't know of an easy fix in rst, so it's fine with me to call it done.

I'm not seeing that in the docs that I've bit manually or in the docs built by the Github actions check. No text is hidden here on my end. Could you provide a screenshot?

I see this in Firefox, but not Chrome, so it looks even less fixable and I still approve the PR.

[hkershaw-brown]

thanks @kdraeder and @mjs2369. Kevin's approved this so I'll bundle it in the next release.

For the text problem, I can't reproduce the TOO LARGE TEXT issue on firefox, chrome, or safari. So I'm going to move this to its own issue, might be browser version, might be local build of docs vs readthedocs, might be something else.

I see this in Firefox, but not Chrome, so it looks even less fixable and I still approve the PR.

Here's firefox with the pull request for me: