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

remove unused, untested functions in utils? #299

Closed
Mikejmnez opened this issue Dec 28, 2022 · 3 comments
Closed

remove unused, untested functions in utils? #299

Mikejmnez opened this issue Dec 28, 2022 · 3 comments
Labels
question Further information is requested

Comments

@Mikejmnez
Copy link
Collaborator

There are, to my knowledge, two unused, untested functions in oceanspy.utils:

  1. find_cs_sn (line 903)
  2. local_to_latlon (line 844)

@MaceKuailv I checked and you wrote them. I think find_cs_sn was written to calculate CS and SN but that was made redundant since cs and sn are provided as output by the model. Is there a reason to keep find_cs_sn? I also remember there were some issues with it but don't remember very well.

The function local_to_latlon, on the other hand, is very relevant. However, aside from having no tests (in test_utils.py), it is actually inconsistent with the staggered C grid. The function is quite short, and basically is:

def local_to_latlon(u, v, cs, sn):
    uu = u * cs - v * sn
    vv = u * sn + v * cs
    return uu, vv

The transformation assumes all u, v, cs and sn are defined at the same point, but they are not. Both CS and SN are defined at center points (Y, X) and thus each need to be interpolated to U and V points.

@MaceKuailv are you using local_to_latlon within oceanspy?

My suggestion:

Have local_to_latlon take the od as argument, so that it can interpolate all of CS and SN along with the respective vector pairs and transform all vector fields at once.

@ThomasHaine
Copy link
Collaborator

Sounds good to me. @MaceKuailv what do you think?

@Mikejmnez Mikejmnez added the question Further information is requested label Dec 28, 2022
@MaceKuailv
Copy link
Collaborator

I think this is very reasonable. I think most models can provide CS and SN.

@Mikejmnez
Copy link
Collaborator Author

closed by #300

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants