bigladder/btwxt

N-dimensional grid interpolation library

btwxt's Issues

Installation is not implemented.

Hi all, @nealkruis , @ptsullivan, @chipbarnaby, @galanCA,

I see that the installation is not implemented. Could you, please, do that?

I can make a MR by myself, but I'm kind of "third party".

Best regards,

Andrii

Message callback should be instance specific

Currently btwxtCallbackFunction is namespace global.

It should belong to class RegularGridInterpolator to allow each instance to report errors to the calling context.

Or perhaps have both. If class level callback is not defined, use the namespace one.

Debug assert in sortedness test (Windows)

The test GridAxis.vector_is_sorted asserts std::is_sorted's "invalid comparator" message on Windows, indicating that the comparator function offered to std::is_sorted() doesn't obey strict weak ordering. Without strict weak ordering, associative containers (set, map, etc) could exhibit undefined behavior, so it's helpful that msvc offers an unobtrusive way to find this potential error (i.e. a debug-build assert).

However, this means that our comparator function [](const double a, const double b) { return a <= b; }) is always going to assert on Windows-debug, because it isn't strict. The "strict" of 'strict weak' means that if you compare a with a (two instances that are equal), the operator should return false. Therefore <= isn't allowed, only <. The "weak" part just means that within a sorted set, you're allowed to have ties between equivalent instances, even if those instances are not equal. (For PODs, of course, equivalence is the same as equality, but it needn't be for all object types.)

The other problem with our comparator function is that, though I can't trace its origin, I suspect that it was not built to be tested, but rather tweaked to make a test pass. We require that a vector with duplicates be rejected as "not sorted." But, as far as strict weak ordering is concerned, something like {1, 3, 3, 5, 9} is sorted. It looks for all the world as if we wrote the predicate to std::is_sorted in order to return the result we wanted for full grid-axis validity, rather than writing a valid predicate just for sort. The fact that a vector with dupes doesn't work as a grid-axis is a different check entirely, but we've convoluted the requirements by naming the checking function "vector_is_sorted()." The function should actually check for both sortedness and duplication, and be called something like vector_is_valid_grid_axis().

I can think of two solutions after fixing the predicate. First, check that the vector is sorted and then check that no adjacent values are equal. Or, convert the vector to a std::set, and see if the sizes of the vector and set are the same.

Handling of NULL/NaN data

pchip method?

Hello,

Thank you for providing in C++ this RegularGridInterpolator similar to what is in scipy.interpolate. That is a feature that I need in one of my code.
However at the moment, I see that the "pchip" method is not implemented. I observe with scipy.interpolate.RegularGridInterpolator that for the applications I aim, the "pchip" method gives much better results (it avoids to create oscillations in flat regions for instance).

Best,

Lucas

More usability and other comments

1. Default names should be shorter to make small string optimization more likely.
2. GridPointDataSet() does not accept a Courier. The day may come when there are possible errors.
3. Data vector sizes are not checked for consistency with grid. In debug, out-of-bounds exceptions occur. In release, nothing. All possible consistency checks should be done during initialization.
4. A more convenient argument order for constructors would be xxxx( name, courier *, blah blah blah). Pass nullptr to get default courier. Motivation: get name and in general error handling are ALWAYS needed (no "real" application would use the default courier). Putting courier at the end means args must be provided where defaults could have been used.
5. Add RGI parent pointers to subobjects. Would allow full context (both RGI and subobject) to be included in error messages.
6. Still too much copying. Make it possible to use caller's data directly, no copying.
7. A case could be made that there are too many constructors. Modest convenience at the cost of confusion. Define a single API and live with it. Simplifies testing.
8. Change g format in error messages so moderately sized values do not use exponential form.

Probably more to come.

Uniform grid searching

Separate enums for calling code clarity

It might be helpful to define some duplicate enums so calling code is more self-documenting.

For example,

Btwxt::GridAxis dbtRange(gridODB, Btwxt::Method::CONSTANT)

would become

Btwxt::GridAxis dbtRange(gridODB, Btwxt::ExtrapolationMethod::CONSTANT)

This would also allow different sets of methods in different contexts. There might be an added interpolation method that cannot be used for extrapolation, for example. Having one enum per context would avoid future breaking changes.

More arrays, less vectors?

There may be ways to use arrays as opposed to vectors. Reduced memory management overhead on caller side?

Cleanup/Performance Comments on Parameters and Returns

I have noticed some improper use of value and reference for parameters and return values. There are multiple instances of each of these and I will point out a few of them in this one umbrella issue.

If an object needs to be passed as an input argument to a function, it should be passed by const reference (or pointer) to avoid making a deep copy. This is true for traditional objects (e.g., struct, class) but also for containers like std::vector and std::string. For example:

typedef void (*BtwxtCallbackFunction)(const MsgLevel messageType, const std::string message, void *contextPtr);

This callback will make a local deep copy of the message argument. And:

double get_value_at_target(std::vector<double> target, std::size_t table_index);

Will make a deep copy of target. That may be the intent here (hard to tell), but if it is then it should be done explicitly with the function by passing target by const & and then doing something like std::vector<double> copy_of_target = target;.

Similarly, objects should be returned by & or const & if possible, i.e., if they are not created within the function and have a lifetime that extends beyond the creating function and their intended use. A good potential example is here:

std::vector<double> get_current_target();

Conversely, scalar arguments should be passed by value. Passing scalars by reference is only needed if the function will overwrite the value. Otherwise, you are just adding dereferences and making life more difficult for the compiler. A good example is here:

 double get_spacing_multiplier(const std::size_t &flavor, const std::size_t &index);

There are a handful of other examples of each kind of problem. Mostly of the first two.

Message handling

Eliminate exceptions or make them optional. One idea is to have the return value of write_message indicate how to proceed. One response could be "Throw".

Replace error() / warning() etc. with messageType

Message format: Often useful to use form: GridAxis 'MyAxis': Linear extrapolation bogus

Messages should be returned as const char* or const string& (not string_view).

Additional thoughts

• Remove context_pointer from Courierr. Given that Courierr is intended to be an interface with the specifics to be provided in derived classes, it seems overthinking-ish to include context_pointer in the base class. Not all derived schemes will need a void*. In particular, I am exploring a template-based derived class that uses a typed pointer, so I am either going to ignore the void* or use it with awkward casting. Let the derived classes provide any needed state storage.
• Change the btwxt throws to logged Errors. Let the derived logging function decide whether to throw. For btwxt, change class BtwxtLogger to do the throw. Default behavior is thus unchanged, but there is more flexibility.
• Perhaps elaborate the intent of the Courierr message types. For example, error() could be documented as assuming the application code will never return. That could actually be enforced with modest restructuring of Courierr -- set things up so if the derived class error() returns, the application aborts.

---

• Review functions that return after logging, if return is meaningless we may want to throw
• Make courierr use const string& instead of string_view
• Document courierr better (example with write_message)
• Add name to RGI for messaging
• Cleaner message wording

Ability to name axes and value tables

Matlab 2d interpolation equivalent

Hello
I am looking for a function in c++ which is is equivalent to:
https://ch.mathworks.com/help/matlab/ref/interp2.html
dose your library has the same ability ?