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

Enable restriction of an even-sphere obs network to a lon-lat box #538

Closed
wants to merge 1 commit into from

Conversation

kdraeder
Copy link
Contributor

@kdraeder kdraeder commented Sep 2, 2023

Description:

This is a new version of even_sphere.m, modified to restrict the observations to a lon-lat box,
but maintain the evenly-spaced pattern produced by the "golden section spiral".
It was needed to create an obs network restricted to the Tropics for a Zagar OSSE.
I generalized it to allow restrictions of longitude as well.
I did some refactoring to reduce the number of similar loops and gather closely-related tasks.

Fixes issue

None (yet)

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.
      The matlab script can print a "help" section, which now has an example of restricting the domain.

Tests

I specified a subdomain that spans the prime meridian and the equator and examined the picture of the profile locations.

Reviewers

I don't know who knows Matlab and would like to review this.

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 kdraeder added Enhancement New feature or request Minor Minor functionality, non-critical data; easy fix obs_converters converting observations to DART format labels Sep 2, 2023
@kdraeder kdraeder self-assigned this Sep 2, 2023
@hkershaw-brown
Copy link
Member

Hi @kdraeder I haven't looked too closely at this yet, but this looks like some added options to even_sphere.m.

Is the goal to replace the existing even_sphere.m so we have one .m file that can creates profiles on the whole sphere, or a restricted horizontal area of the sphere?

@kdraeder
Copy link
Contributor Author

kdraeder commented Sep 6, 2023

It could be a replacement, but I saw that there is already a variant; even_sphere_pe2lyr.m.
So I thought maybe the philosophy here might be to have specialized scripts.
There are no log entries for even_sphere_pe2lyr.m, so it's not clear.
Generally I'm in favor of minimizing code and having fewer programs that do more things,
but I've been outvoted in the past. So it's your call.

@nancycollins
Copy link
Collaborator

based on the lack of capitals in the comment at the top of the pe2lyr, i'd guess it's something i made. i'd vote to remove it from the repo. it's just an older duplicate of the basic program with only 2 levels in the vertical. it's not worth keeping, in my view. as long as the code for a restricted lat/lon box doesn't complicate the program so much that it's hard to follow, i'd vote for a single program here.

default_lat_south = -90.;
default_lat_north = 90.;

if (exist('inputParser/addParameter','file') == 2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

it would help if you could add a comment about why one section needs to use addParameter and the other needs to use addParamValue - it's a matlab thing i guess but it's not clear to me what the diff is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know the difference either. I was just following the template.
I'd need to dig into Matlab to figure it out. I'll do it if it's important.

num_plot = 0;
for k = 1:nprofiles
% Input is in degrees of latitude and longitude; convert from radians
lon_deg(k) = rad2deg(lon(k));
Copy link
Collaborator

Choose a reason for hiding this comment

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

so lon_deg and lat_geg seem to be global, and plot_lon and plot_lat are box bounded. but when you're writing out the obs, they seem to be using the lon_deg/lat_deg global values? (line 245). am i missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

am still curious if this is a bug or if i'm not reading the code right. when you used this did you only generate obs inside the specified box? (or maybe not of interest anymore if we're going with the obs_sequence_tool extraction route.)

@nancycollins
Copy link
Collaborator

if all the box does is restrict the output to that box and doesn't change where the points are generated, then the obs_sequence_tool has a box option to cull obs outside the box. you could run the original code unchanged and then cut the obs down as a post processing step.

@hkershaw-brown
Copy link
Member

@kdraeder let's put your changes into even_sphere.m
Leave even_sphere_pe2lyr.m as out of scope for this pull request

git mv even_sphere_subdom.m even_sphere.m should replace even_sphere.m with your new changes.

@kdraeder
Copy link
Contributor Author

kdraeder commented Sep 6, 2023

@nancycollins comment about obs_sequence_tool reminds me that in Manhattan
we'll be using the preprocessor strategy for cam. I'd forgotten that, as I was importing
this from the reanalysis branch where I needed it and there's no preprocessor step.
Creating the global obs set and extracting a subset using obs_sequence_tool
in the preprocessor step looks like the more consistent strategy.

So it appears to me now that we should leave even_sphere.m unchanged,
ignore even_sphere_subdom.m, and close this PR as "won't do" (?).
Sorry for the wasted effort.

@hkershaw-brown
Copy link
Member

@kdraeder no problem, do you want to switch the PR from main to the reanalysis branch to have a record of what what used?

@nancycollins
Copy link
Collaborator

even_sphere used to be a simple matlab script. now it's a function - but you have to add a bunch of arguments to get it to run at all? that doesn't seem good. can't there be reasonable defaults in the script? i can't figure out how to run it at all. we should at least have an example matlab script that calls it with reasonable values. someone who knows too much matlab made this pretty hard to use, in my opinion. </end rant>

@nancycollins
Copy link
Collaborator

ok - i found you can run it with even_sphere(100) to gen 100 obs. but i tried restricting it with the lat/lon box and it didn't generate obs inside the box. the plot looks like the box you specified, but the locations in the output file aren't the same. so we definitely don't want this version on main, and if this isn't the exact version kevin used, you don't want it on the reanalysis branch either.

@kdraeder
Copy link
Contributor Author

kdraeder commented Sep 6, 2023

Agreed. It's broken. It's not needed. I'm abandoning it.

@jlaucar
Copy link
Contributor

jlaucar commented Sep 6, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request Minor Minor functionality, non-critical data; easy fix obs_converters converting observations to DART format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants