Adding ncor (Coriolis) to the param.nml file

[BahramKhazaei-NOAA]

I was wondering why the Coriolis option (ncor) is commented out from the opt function? I believe it is an important parameter to be added to the default/template namelist that comes from pyschism pre-processor.

[SorooshMani-NOAA]

@BahramKhazaei-NOAA, I just realized what you meant by "the commented lines" after looking at the file you referenced above. I think that part of the commented code is from the old implementation, but the Coriolis value still can be set using param.ncor.

Depending on how you would setup using PySCHISM, at some point you get a Param object. e.g. if you create a coldstart driver using ModelConfig you get the param object from the coldstart object like:

config = ModelConfig(...)
.
.
.
coldstart = config.coldstart(...)
param = coldstart.param

so you can set the value by:

coldstart.param.ncor = <the value of interest>

I'm not sure if this fully resolves your issue, but since I thought it's relevant I thought I'd mention it here.

[BahramKhazaei-NOAA]

That's how I do it right now, but it would be nice if the ncor is written to the parm.nml by default. It can have a value of zero, but I think it should one of the parameters existing in the minimal namelist file.

[SorooshMani-NOAA]

Oh, I see what you mean. Yeah, it makes sense

[BahramKhazaei-NOAA]

@cuill Any plans/updates on this? I saw a few people recently with the same issue when they used pyschism for the preprocessing of their model inputs. Thank you very much.

[cuill]

@BahramKhazaei-NOAA Can you try branch bugfix/param to see if it works?

[BahramKhazaei-NOAA]

@BahramKhazaei-NOAA Can you try branch bugfix/param to see if it works?

@cuill Thank you for your quick response. @FariborzDaneshvar-NOAA could you please try the branch Linlin suggested above?

[SorooshMani-NOAA]

I think instead of each person testing it, it makes more sense to add a simple test case in pyschism repo and also activate the tests for future commits.

@FariborzDaneshvar-NOAA doesn't use pyschism directly right now, so it doesn't make sense for him to be involved in testing this.

With that being said, what is it that we want to test exactly? Right now the change I see is that by default, when the grid is geographic (ics = 2) then ncor will be set to 1, otherwise to 0. Still if it is not geographic, it won't write out ncor = 0 to the param.nml.

So going back to my questions, what is it that we want to have and hence what is it that we need to test. When we know the answer, let's add it to the pyschism tests to run on GitHub at every commit.

@cuill I'd be happy to help with adding this first test there.

[BahramKhazaei-NOAA]

I think instead of each person testing it, it makes more sense to add a simple test case in pyschism repo and also activate the tests for future commits.

@FariborzDaneshvar-NOAA doesn't use pyschism directly right now, so it doesn't make sense for him to be involved in testing this.

With that being said, what is it that we want to test exactly? Right now the change I see is that by default, when the grid is geographic (ics = 2) then ncor will be set to 1, otherwise to 0. Still if it is not geographic, it won't write out ncor = 0 to the param.nml.

So going back to my questions, what is it that we want to have and hence what is it that we need to test. When we know the answer, let's add it to the pyschism tests to run on GitHub at every commit.

@cuill I'd be happy to help with adding this first test there.

Soroosh, thank you for sketching a plan for testing. I believe that will make sense. My only recommendation with respect to adding ncor to namelist file output of the pyschism is that we should have the ncor parameter as one of the defaults written to a param.nml file. It can be ncor = 0 so at least it comes to the attention of the user when they get poor simulated results related to Coriolis.

[SorooshMani-NOAA]

@BahramKhazaei-NOAA I remember we discussed this before, that's why I wanted to point out that the updated code does not do exactly that; it changes the pyschism.ModelDriver's "default" value of ncor to 1 when we use geographic grid (which is most of the cases maybe) and hence writes it to param.nml (since 1 is different from the SCHISM default of 0).

But still if we set the value to 0 it won't write it to param.nml.

If this is good enough, we can add a test for this changed behavior, but still it's different from what we talked about before!

[BahramKhazaei-NOAA]

@BahramKhazaei-NOAA I remember we discussed this before, that's why I wanted to point out that the updated code does not do exactly that; it changes the pyschism.ModelDriver's "default" value of ncor to 1 when we use geographic grid (which is most of the cases maybe) and hence writes it to param.nml (since 1 is different from the SCHISM default of 0).

But still if we set the value to 0 it won't write it to param.nml.

If this is good enough, we can add a test for this changed behavior, but still it's different from what we talked about before!

I personally don't have a strong opinion about the ncor value (actually I believe ncor = 1 is more common that 0 but in that case we should make sure that coordinate option is adjusted accordingly, i.e., ics =2 ). My only recommendation is that ncor should be present as one of the default parameters in the minimal namelist file that comes out of pyschism.

[cuill]

@SorooshMani-NOAA Thanks for testing and your suggestion.

Before I dig into class Param, I would like to bring up how differently we deal with param.nmlfile. We just take a well-tuned param.nml file and change the basic parameters (e.g., startdate, rnday) for each run. In my opinion, it is not worth putting much effort into developing this semi-static file. That being said, maybe we can think of a way to minimize class Param.

Let me know what you think.

[SorooshMani-NOAA]

Actually I take part of my statement back! The Param class ignores writing parameters that are not set, indicated by being equal to None so the new code also writes 0. I'll write tests to make sure I'm not missing anything again!

@cuill I understand the difference in approach VIMS is taking for setting up the model, but being on a less SCHISM-technical side, we'd like to develop a param.nml from scratch, and having some of the parameters not hidden in that from-scratch version is also useful.

I'll write the tests for this and then will pass it to you to add to your changes before merging. I'm not sure about enabling the auto-testing on GitHub yet, that might need more time, so I'll leave that part for later.

[SorooshMani-NOAA]

@cuill please see the tests I added in https://github.com/SorooshMani-NOAA/pyschism/tree/bugfix/param. This branch is based off of your bugfix/param so it has your fixes.

To run the test pytest should be installed and you can call:

pytest tests/test_ncor.py

This along with the current existing tests can be added to the automated pipeline at some point. But in general it's a good idea to add tests as we update behavior or make bugfixes. At least we can run it manual to make sure these work after each update.

[cuill]

@SorooshMani-NOAA Thanks for contributing to the test scripts.

It gave me an error in package cfunits:

[viz] pytest test_ncor.py 
================================================== test session starts ===================================================
platform linux -- Python 3.8.10, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
rootdir: /sciclone/data10/lcui01/forecast_3D
plugins: anyio-3.7.1
collected 0 items / 1 error                                                                                              

========================================================= ERRORS =========================================================
_____________________________________________ ERROR collecting test_ncor.py ______________________________________________
test_ncor.py:7: in <module>
    from pyschism.mesh import Hgrid
../pyschism/pyschism/__init__.py:4: in <module>
    from pyschism.stations import Stations
../pyschism/pyschism/stations.py:12: in <module>
    from pyschism.enums import (
../pyschism/pyschism/enums.py:707: in <module>
    class Sflux1Types(Enum):
../pyschism/pyschism/enums.py:709: in Sflux1Types
    from pyschism.forcing import nws
../pyschism/pyschism/forcing/__init__.py:3: in <module>
    from pyschism.forcing.nws.nws2.gfs import GlobalForecastSystem
../pyschism/pyschism/forcing/nws/__init__.py:1: in <module>
    from pyschism.forcing.nws.nws2 import NWS2
../pyschism/pyschism/forcing/nws/nws2/__init__.py:1: in <module>
    from pyschism.forcing.nws.nws2.nws2 import NWS2
../pyschism/pyschism/forcing/nws/nws2/nws2.py:9: in <module>
    from pyschism.forcing.nws.nws2.sflux import SfluxDataset
../pyschism/pyschism/forcing/nws/nws2/sflux.py:9: in <module>
    import cf
../../../home20/lcui01/anaconda3/lib/python3.8/site-packages/cf_python-3.8.0-py3.8.egg/cf/__init__.py:155: in <module>
    import cfunits
../../../home20/lcui01/anaconda3/lib/python3.8/site-packages/cfunits-3.3.1-py3.8.egg/cfunits/__init__.py:46: in <module>
    from .units import Units
../../../home20/lcui01/anaconda3/lib/python3.8/site-packages/cfunits-3.3.1-py3.8.egg/cfunits/units.py:2480: in <module>
    class Utime(cftime.utime):
E   AttributeError: module 'cftime' has no attribute 'utime'

[SorooshMani-NOAA]

@cuill no problem! I think this has something to do with your environment. The error happens at from pyschism.mesh import Hgrid, so something is not installed properly on your environment. Just to be sure, open a python session and try to just call the same from pyschism.mesh import Hgrid and see if you get the cfunits error. If you do, it's just the environment packages.

Maybe something needs to be added to the setup.py as dependency