search

xgcm / aerobulk-python

A python wrapper for aerobulk (https://github.com/brodeau/aerobulk)
GNU General Public License v3.0
14 stars 4 forks source link

Streamline python API #5

Closed jbusecke closed 2 years ago

jbusecke commented 2 years ago

The implementation in #3 is currently working on my local machine, but the python API is a bit wonky.

You currently have to do: 

import aerobulk.mod_aerobulk_wrap as aero
aero.mod_aerobulk_wrapper.aerobulk_model_noskin(...)

See also here.

We can change this after the package is working on conda-forge, just wanted to start a discussion on what we would like the API to be.

cc @rabernat @TomNicholas @paigem @dhruvbalwada @cisaacstern

jbusecke commented 2 years ago

Based on our meeting today I would like to explore options for the API.

We are currently only using a subset of the functionality of aerobulk, but it might be worth considering how to add the full functionality later.

As far as I can see the main thing to consider here is that some (but not all) algos have an option to compute skin_temperature, which requires additional input arguments, and returns additional output arguments.

The way I see it we could either have two high level functions: 

from aerobulk import calc_flux_noskin, calc_flux_skin

a,b,c = calc_flux_noskin(sst, hum, temp, u, v, zt=10, zu=2)
a,b,c, d = calc_flux_noskin(sst, hum, temp, u, v, additional_input, zt=10, zu=2)

Or have one wrapper function which returns different amounts of outputs depending on the input

from aerobulk import calc_flux
a, b, c = calc_flux(sst, hum, temp, u, v, zt=10, zu=2, skin_temperature=False)
a, b, c, d = calc_flux(sst, hum, temp, u, v, zt=10, zu=2, additional_input=..., skin_temperature=True)

I think I prefer the former, but would like to hear opinions from @rabernat, @TomNicholas, @paigem.

rabernat commented 2 years ago

I also prefer the former. It doesn't make sense to cram two different sets of inputs and outputs into one function.

jbusecke commented 2 years ago

Yeah agreed. In terms of naming (which I am notoriously bad at), how do we feel about calc_flux_skin/calc_flux_noskin?

jbusecke commented 2 years ago

Took a first shot at this in #8, feedback very welcome.

jbusecke commented 2 years ago

You can see the new API in action in this line: https://github.com/jbusecke/aerobulk-python/blob/1794f928f1da8ea8661f08819e7867c6a9c0fcdb/test/test.py#L24