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

Highcam #422

Closed
wants to merge 17 commits into from
Closed

Highcam #422

wants to merge 17 commits into from

Conversation

nancycollins
Copy link
Collaborator

@nancycollins nancycollins commented Nov 4, 2022

Description:

Adds a new program to remove obs above a given CAM model level from an obs_seq file. This program takes a single obs_seq file in and writes a single obs_seq file out without the high obs. In the future it could also superob dense obs, change obs errors near the surface, or any other CAM-specific processing that might be needed.

It can either be added to the cam workflow scripts, or a month of obs files could be run offline ahead of time and then the cam scripts can read in the updated files instead of the originals.

Edit: this has been moved to pull request #424
Also added an unrelated update to the clean_nml.c program to capture additional features i've added to it. Probably should be moved to its own pull request but it's a single file update so i dumped it here.

Fixes issue

#415

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

Ran with one of the reanalysis obs_seq files and it removed the high obs.

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

kdraeder and others added 10 commits February 2, 2022 20:07
Main motivation was to get the CAM-SE branch.
assimilating obs at the cam model top leads to problems because
the model top has a fixed upper bound.  this program uses the routine
inside the cam model_mod.f90 to remove obs above a given model level.
add some command line args to control how much to sort (all contents
of nameslists but leave the namelists in the original order, vs sorting
the namelists themselves into alphabetical order), and whether to remove
comment lines outside of namelists or not.
@hkershaw-brown
Copy link
Member

@nancycollins I am going to split this into two pull requests (clean_nml and high cam) and then ask you a bunch of questions on both. Stay tuned

@kdraeder
Copy link
Contributor

kdraeder commented Nov 7, 2022

@nancycollins @hkershaw-brown
What's the relationship between the proposed highcam and the recently created camdart_obs_preprocessor?
(#296 and #415)
On the surface it seems that highcam may end up doing what I thought preprocessor is intended to do,
but has a less generic name.

@hkershaw-brown
Copy link
Member

clean_nml moved to pull request #424

@nancycollins
Copy link
Collaborator Author

@nancycollins @hkershaw-brown What's the relationship between the proposed highcam and the recently created camdart_obs_preprocessor? (#296 and #415) On the surface it seems that highcam may end up doing what I thought preprocessor is intended to do, but has a less generic name.

highcam is the name of the git branch with the camdart_obs_preprocessor code in it.

trim(filename_out)
call error_handler(E_MSG,'main',msgstring,source)

print*, 'Number of obs in the output seq file :', get_num_key_range(seq_out)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the different methods for printing messages? (print and error_handler(E_MSG...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

line 271 looks like a debugging line that should be removed. i'm also a bit mystified about the "if not verbose" test at 260. it looks like that should be "if verbose". i think the idea was verbose messages might print to the screen but other messages go into the log as well as screen. my suggestion is to run it a couple times with and without verbose and decide what you think are the most useful messages for various situations - like if the output file is empty, what info would help to print out? and put those prints in the verbose sections. and if you want to use msg for everything, i'm ok with that too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've made some changes, which I'll push for people to check out.
I went a bit beyond what's been suggested so far.

| | | |
+--------------+--------------------+--------------------------------------------------------------------------------+
| calendar | character(len=32) | The string name of a valid DART calendar type. (See the |
| | | :doc:`../../modules/utilities/time_manager_mod` documentation for a list of |
Copy link
Contributor

@kdraeder kdraeder Nov 18, 2022

Choose a reason for hiding this comment

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

links to other documentation look like ':doc:`../../../assimilation_code/'...
in the github rendering. Will it look different in the docs.dart.ucar.edu?

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'm going to leave the doc situation to others - i haven't done it enough to understand the current process, sorry.

Copy link
Contributor

@kdraeder kdraeder left a comment

Choose a reason for hiding this comment

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

@nancycollins @hkershaw-brown
I've made a few comments and suggestions.
None of them are worrisome.

@nancycollins
Copy link
Collaborator Author

hi kevin, thanks for the review - all good points. if you're willing to address all or some of them, i'd love for you to be the one to take ownership of this program since it's possibly going to be needed for more functions in the future. if you're busy or don't want to address some, email me and i'll look at doing some of the updates. thanks.

@kdraeder
Copy link
Contributor

hi kevin, thanks for the review - all good points. if you're willing to address all or some of them, i'd love for you to be the one to take ownership of this program since it's possibly going to be needed for more functions in the future. if you're busy or don't want to address some, email me and i'll look at doing some of the updates. thanks.

I agree that it makes sense for me to look after it.
I'll work through the name change, leave the "use ..." contents (I hadn't thought of expansion),
and look into whether I can fix the others.

@kdraeder kdraeder self-assigned this Nov 21, 2022
and fixed typos in the previous commit (camdart -> cam_dart changes).
Copy link
Member

@hkershaw-brown hkershaw-brown left a comment

Choose a reason for hiding this comment

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

What is up with these merges from the CAM-SE branch?

cam_dart_obs_preproessor is not in the table of contents for the documentation.

@hkershaw-brown
Copy link
Member

We can talk about this after thanksgiving, but this merge history is weird.

When working with a pull request you can checkout the feature branch

https://ncar.github.io/dart-developers-guide/reviewing.html

Don't accept this pull request as is.

@kdraeder
Copy link
Contributor

I wasn't aware of the reviewing.html. Thanks for the tip. I'll try to understand it before any more github work.

I'm working in /glade/u/home/raeder/DART/Manhattan_git.
This was updated some weeks ago to have the cam-se shared code in it.
I believe that I checked out Nancy's qc8fix branch to do that.
I did some work with that, cleaned it up, then checked out main.
I made a highcam directory, went there, and
$ git pull origin highcam
I made a bunch of changes to the preprocessor, as discussed above, committed them, then
$ git push origin highcam:highcam
Apparently this was misguided.

@hkershaw-brown
Copy link
Member

moved commits to high-cam

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.

None yet

3 participants