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

Refactoring code base #106

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactoring code base #106

wants to merge 1 commit into from

Conversation

haakon-e
Copy link
Member

@haakon-e haakon-e commented Jul 3, 2021

I refactored the code base to more "modern" python, including:

  • getting rid of most try/except clauses (bad style)
  • getting rid of empty return statements
  • raising errors instead of printing them or calling sys.exit
  • use dictionaries to define cases instead of if/else clauses, except where nogil is used
  • use .get(key, default) on dictionaries to get values and provide a default if key not found
  • use pass instead of empty return in empty functions
  • optimize some code by disabling bounds checks on indices
  • specify language level on compilation

this effectively assumes you have a recent-ish version of Python (e.g. definitely not Python 2.x), and my hope is makes the code somewhat easier to read and accessible to new users.

Most important feedback on this PR is
a) did I inadvertently change any numeric values of any parameters, etc.?
b) do you have any disagreements in the proposed changes?
c) does the code also compile on your machine (it does for me)?

PS: I made these changes on a day I was otherwise feeling unproductive, so please don't make reviewing this a priority if you have better things to do. This PR can definitely sit around for a few weeks/months without problem.

@trontrytel
Copy link
Member

Hi @haakon-e . Thanks for doing this!

I tried running one of our tests by

$ py.test -s -v plots/test_plot_TRMM_LBA.py

in the tests folder and I'm getting:

request = <SubRequest 'sim_data' for <Function test_plot_TRMM_LBA>>

    @pytest.fixture(scope="module")
    def sim_data(request):
    
        # remove netcdf file from previous failed test
        request.addfinalizer(cmn.removing_files)
        # generate namelists and paramlists
        setup = cmn.simulation_setup('TRMM_LBA')
    
        # run scampy
        subprocess.call("python3 setup.py build_ext --inplace", shell=True, cwd='../')
>       scampy.main1d(setup["namelist"], setup["paramlist"])

plots/test_plot_TRMM_LBA.py:27: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../main.py:29: in main1d
    Simulation = Simulation1d.Simulation1d(namelist, paramlist, inpath)
Simulation1d.pyx:23: in Simulation1d.Simulation1d.__init__
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   KeyError: 'casename'

Cases.pyx:32: KeyError
=============================================================== short test summary info ================================================================
ERROR plots/test_plot_TRMM_LBA.py::test_plot_TRMM_LBA - KeyError: 'casename'

Do the tests work for you after the refactor?

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.

2 participants