EUREC4A-UK / lagtraj

Python trajectory code for Lagrangian simulations
MIT License
11 stars 9 forks source link

conversion for SAM LES #160

Open xychen-ocn opened 2 years ago

xychen-ocn commented 2 years ago

Major additions:

Minor correction:

leifdenby commented 2 years ago

this is looking good @xychen-ocn!

To fix the linting issues you just need to run the linting fixers (black, flake8, etc), but that can easily be done with pre-commit. I wrote some development notes on how to get that set up: https://github.com/EUREC4A-UK/lagtraj/blob/master/docs/developing.md

In those notes you'll also find out how to the run the tests locally on your computer. You can see that a few of the tests are failing right now: https://github.com/EUREC4A-UK/lagtraj/runs/5831242140?check_suite_focus=true#step:6:987 I'm not sure about the error for the kpt forcing, maybe @sjboeing you have thoughts on this?

xychen-ocn commented 2 years ago

Hi @leifdenby, I encountered some issue with black but not sure how to fix it. The problem is the same as this issue posted here: https://github.com/psf/black/issues/2964 As suggested therein, I tried making click==8.0.4 and 8.0.1 but it was not working for me. :( Also, I had an error saying "sphinx-external-toc 0.2.2 requires click~=7.1", I am not sure if that is why the above workaround doesn't work for me.

leifdenby commented 2 years ago

The problem is the same as this issue posted here: psf/black#2964

Yes, sorry! It looks like something in the click package broke something inside of black. I'll make a pull-request to change the version of black we use to fix this!

leifdenby commented 2 years ago

I've updated master with the changes for black now, hopefully that should fix it! You just need to pull in the changes

xychen-ocn commented 2 years ago

@leifdenby Thanks for the quick update! This branch has passed the linting now. I reverted my units change for sdor in kpt.py because that didn't pass the local pytest. So the only changes are sam related. I will check why I had that problem with "sdor" a while ago, maybe I need to reinstall lagtraj to match your lastest published version.

leifdenby commented 2 years ago

I will check why I had that problem with "sdor" a while ago, maybe I need to reinstall lagtraj to match your lastest published version.

If you add the main repository as a "remote" with git you will be able to pull down the changes (convention is to call this "upstream"):

git remote add upstream https://github.com/EUREC4A-UK/lagtraj
git pull upstream master

That way you'll get the most recent changes into the branch you're working on locally :)

leifdenby commented 2 years ago

Also, in case you hadn't spotted it (and if you're running the tests locally): I added some points at the end of the development notes on how to speed up running the tests: https://github.com/EUREC4A-UK/lagtraj/blob/master/docs/developing.md#speeding-up-testing, the main thing is to download the testdata manually yourself and point to it so that the test routine doesn't do a download every time you run the tests :smile:

NB: I've just noticed I need to update those notes because the tar.gz-file with the testdata has changed over time. The one you'll need is http://gws-access.jasmin.ac.uk/public/eurec4auk/testdata/lagtraj.testdata.v0.1.0.tar.gz, you can always look in tests/conftest.py where there test-data url is included (I'm going to update the development notes with this).

sjboeing commented 2 years ago

Good work @xychen-ocn! I just left a few notes on what we are doing for the current release, but hope to have another look at this soon.

xychen-ocn commented 2 months ago

Hi @sjboeing and @leifdenby,

% get surface variables
  in.Ps = in.presh(end,:);              % Pressure at half level.
  for k = 1:Ntime
    in.q_surf(k) = interp1(in.pres(:,k),in.q(:,k),in.Ps(k),'linear','extrap');
    in.T_surf(k) = interp1(in.pres(:,k),in.t(:,k),in.Ps(k),'linear', 'extrap');
  end

and so I did the same thing in sam.py (L378 - L396).

Maybe we could discuss together with Peter (@pblossey) if there is a better approach. Perhaps the extrapolation should be bounded by the skin temperature and the surface saturation specific humidity? Or perhaps as Steef suggested, we use the skin temperature as in kpt format, and then the equivalent concept for specific humidity?

xychen-ocn commented 2 months ago

This looks great to me @xychen-ocn - thanks for doing this!

To my mind the only remaining bits would be to

  1. add "SAM" to the README (https://github.com/EUREC4A-UK/lagtraj/blob/master/README.md#forcing-profiles-conversion-targeting-a-specific-gcmles). Maybe you could add a link too like we have for KPT and dephy?
  2. also make an addition to the CHANGELOG with the feature you've added :)

For the changelog you could add something like

# Changelog

## [Unreleased](https://github.com/EUREC4A-UK/lagtraj/tree/HEAD)

[Full Changelog](https://github.com/EUREC4A-UK/lagtraj/compare/v0.1.1...HEAD)

*features*

- Add support for converting forcing files to target the SAM LES model
  [\#160](https://github.com/EUREC4A-UK/lagtraj/pull/160)
  @xychen-ocn 

Feel free to change that text for the changelog, you could put in your full name and email too if you'd like

Somehow I missed this message. I have now made changes to these two files as suggested!