-
Notifications
You must be signed in to change notification settings - Fork 54
Description
Personally, I've never been a huge fan of the rate functions API. I'm not opposed to exposing this kind of functionality, but I think that the current approach of exposing every rate as a distinct function leaves a lot to be desired.
I know that this API was the simplest thing for us to do; the API let us expose internal logic to users. While I'm not fundamentally opposed to that kind of approach (after all, we aren't full-time software engineers), I think there are way too many of these functions to ignore the warts (after we merge in the newchem-cpp branch there will be 80+ collisional rates)
Concerns about the current API
My concerns about the current API fall into 2 categories:
- "Mechanics:"
- I don't like that we are polluting the namespace of all c functions. Names like
k13_rate
orbrem_rate
are terribly generic, and its plausible we could produce name collisions.1 - Ideally, we would probably be explicitly unit-testing (or performing basic smoke tests) on every public function.
- We may need to stop passing
chemistry_data
as an argument (I think that the 4.0 release may replace thechemistry_data
type)
- I don't like that we are polluting the namespace of all c functions. Names like
- "Ergonomics:"
- it's really easy to "shoot yourself in the foot." To know the proper units argument you need to explicitly read the documentation
- the ergonomics of having 100+ functions is pretty terrible. If somebody wants to call every function, they need to write out 100 different function names in their codebase (and they need to figure out what the proper unit is for every single function)
- I would argue that the tediousness of the current API is exemplified by the fact that we don't bother to explicitly list out every single function in our own API documentation
Thoughts on how to improve the API
I don't have all the full answer. But I might have a good way to start.
We could replace a lot of individual functions with one of the following
-
a single function that does the calculation for a lot of different rates
int gr_calc_collisional_rate(const char *rate_name, double* out, unsigned int length, const double* T, double kunit, chemistry_data* my_chemistry);
where
rate_name
holds the name of the rate.2 The idea is that this call would store values inout
. -
a function that returns a function pointer, represented by the
rate_function
type,3 that the end-user would invoke to compute rates.rate_function gr_get_rate_fn( const char *rate_name );
Since this will be defined in a C++ file, we will probably need to declare each of the functions returned as function pointers as
extern "C"
(if we want this function to be usable in C libraries).4
Of course, we can add functions to do lots of other things (e.g. query the kind of units associated with a given rate, query reactants/products in a rate, ...). It all boils down to the intended use case for these functions, which has never been very clear to me.
If these functions are only intended for experimentation, it may make sense to explicitly make them part of an unstable API (so we have the freedom to change the functions between versions).
Footnotes
-
If this were the ONLY problem, we could resolve it by simply changing the names to something like:
gr_k13_rate
gr_brem_rate
. ↩ -
We could replace
chemistry_data
with the appropriate type if it is removed in 4.0. ↩ -
If performance is important, we probably want
rate_function
to compute values at several temperatures in a single call. ↩ -
Ideally, we wouldn't make any of names of the functions, that are returned as pointers, publicly visible to external code (i.e. the only way to call them is through a pointer returned by
gr_get_rate_fn
-- you can't simply specify the name). The most convenient way to do that is to declare them asstatic
. Unfortunately you can't declare a function as bothstatic
and asextern "C"
. So we may want to replacestatic
with clang/gcc attributes for controlling visibility (or we can live with the symbols being visible). ↩