Loss function clarity #28
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #28 +/- ##
==========================================
+ Coverage 83.52% 84.15% +0.62%
==========================================
Files 28 28
Lines 680 713 +33
Branches 71 71
==========================================
+ Hits 568 600 +32
- Misses 92 93 +1
Partials 20 20
Continue to review full report at Codecov.
|
|
|
||
| import numpy as np | ||
| from scipy.interpolate import interp1d | ||
|
|
||
| from ..aliases import ArrayOfParameterVectors, Weights | ||
| from ..aliases import ArrayOfParameterVectors, LossFunction, ParameterVector, Weights | ||
| from ..geometric import _norm_of_arrayofparametervectors |
There was a problem hiding this comment.
Since you are using _norm_of_arrayofparametervectors in a different module than it comes from (i.e. in elastic_band and not in geometric, I think it makes more sense to not make it private?
Not 100% sure though, thoughts @alexjuda?
There was a problem hiding this comment.
That's a good point. It is only a helper function for us though. I doubt that anyone would need that apart from us.
There was a problem hiding this comment.
I can recommend a 3-fold distinction for project symbols – public/internal/fileprivate based on a convention with leading underscores in the module and symbol names;
- public:
module,function - internal:
_module,function - fileprivate: w/e,
_function
The above use case looks like an "internal" one :). Thoughts?
There was a problem hiding this comment.
@alexjuda In our case the content of geometric packages should be public, so not really ;)
@MSRudolph fine 🤷♂️
| @@ -148,3 +155,11 @@ def uniformly_distribute_trajectory( | |||
| ) | |||
| eval_points = np.linspace(0, 1, num=n_points) | |||
| return weight_interpolator(eval_points) | |||
|
|
|||
|
|
|||
| def _norm_of_arrayofparametervectors(param_array: ArrayOfParameterVectors): | |||
There was a problem hiding this comment.
Not sure about the name of the function...
Thoughts, @alexjuda ?
There was a problem hiding this comment.
How about just def _norm(param_array: ArrayOfParameterVectors):?
phrasing Co-authored-by: Michał Stęchły <michal.stechly@gmail.com>
…uting/orqviz into loss-function-clarity
Goals of this draft PR:
np.ndarrayrather than strictly a 1Dnp.ndarrayfunctools.partialloss_function_kwargsin the scanning functions that we pass to the loss_function with more than one argument.