Create a way to encode general functions

[NoureldinYosri]

No description provided.

[wcourtney]

Can you provide documentation for what an "interpolation method" does and how a client should evaluate the options for values?

[wcourtney]

Can you add documentation for the expected behavior of "interp"? We should not depend on a python library (numpy) as the description so that we are not bound to its implementation.

[NoureldinYosri]

added

[wcourtney]

If there's only one possible value, should we omit this field until there are other options?

[NoureldinYosri]

ommitted

[wcourtney]

IIUC, these fields are meant to evoke y = f(x)?

Rather than calling a field x and adding a comment to document that it's "the independent variable", WDYT of calling the field something like independent_variables to be self-documenting? Note that the suggestion is pluralized to follow the style guidance for repeated fields: https://protobuf.dev/programming-guides/style/#repeated-fields

[NoureldinYosri]

renamed

[wcourtney]

Please document what the string keys in this map represent.

[NoureldinYosri]

done

[wcourtney]

How do gate_args differ from custom_args? i.e. could FunctionInterpolationData be added to the existing Arg to support use of the existing gate_args? If not, is there a clear guidance (and documentation) for whether a new argument type should be an Arg vs a CustomArg?

[NoureldinYosri]

How do gate_args differ from custom_args?

Arg are values that we can use as is. That's after deserialization we can use that value internally. CustomArgs are args that we need to perform extra processing on before we can use.

could FunctionInterpolationData be added to the existing Arg to support use of the existing gate_args?
no

---

I think if we just add an extra possiblitiy to Arg we will just increase the complexity of (de)serialization with no obvious gain.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.85%. Comparing base (fd95547) to head (e5c21d6).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6788   +/-   ##
=======================================
  Coverage   97.85%   97.85%           
=======================================
  Files        1084     1084           
  Lines       93590    93651   +61     
=======================================
+ Hits        91580    91642   +62     
+ Misses       2010     2009    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[NoureldinYosri]

@wcourtney ptal

[pavoljuhas]

Can we add a check if x and y have matching shapes?

Suggested change

	- `x` is 1D and not sorted in increasing order.
	- `method` is not supported.
	- `x` is not sorted or has 2 or more dimensions.
	- `y` has a different shape than `x`

[NoureldinYosri]

fixed docstring.

[pavoljuhas]

Please use assignment instead of extend() to set message arrays.

[pavoljuhas]

Apologies @NoureldinYosri, @wcourtney about bike-shedding, but calling these fields x_values, y_values, would perhaps make it a bit easier to parse what these are; it would also follow the recommended pluralization guideline.

How about

Suggested change

	// The independent_var must be sorted in ascending order.
	// The independent_var and dependent_var must be of the same length.
	repeated float independent_var = 1 [packed = true]; // The independent variable.
	repeated float dependent_var = 2 [packed = true]; // The dependent variable.
	// The x_values must be unique and sorted in ascending order.
	// The y_values must be of the same length as x_values.
	repeated float x_values = 1 [packed = true]; // The x values where the function is sampled.
	repeated float y_values = 2 [packed = true]; // The function values at the corresponding x_values.

[pavoljuhas]

The name may suggest the function itself, such as np.sin, should be passed.
How about renaming to function_values_to_proto?

If we have this one, consider adding a convenience counterpart function_values_from_proto to extract x, y arrays.