-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Weights for modelfit could be more intuitive #149
Comments
Not in the traditional sense, but its behaviour is functionally equivalent to weights.
it's not intended for use without the helper function. In fact, I wouldn't even call it a helper function, it is the function intended for anyone who wants to weight their cost. This may be an issue of documentation rather than implementation. The only docs related to this, apart from the API, is the 'in progress' page at https://juliarheology.github.io/RHEOS.jl/stable/numerical/ Is the
Sounds good to me. I'm not convinced that speed-up will be significant without further benchmarking, but if it is (subjectively) more intuitive to have an array of floats then I can't think of any drawbacks right now. However, bear in mind the history of why I ended up adding this approach, it was a direct workaround for the previous approach which was to resample. We discussed this in issue #14 - would be good to also check with @Ab2425 if there are reasons to do it the current way instead of the cost-float-multiply method you suggested. |
Thanks @moustachio-belvedere . I did discuss it briefly with @Ab2425 this morning. It was useful indeed to be reminded the background. Speed up will of course depend a lot on the function that is evaluated, and differences may not be visible at all in practical situations. But considering the summation alone for calculating the cost, I'd expect a significant speed-up with a dot-product and less allocation. We could even reuse
For sums, there are also higher precision methods if needed. Just placing this here for the record: I appreciate that weights were supposed to be used with the helper function, and I generally like the principle of it. But it's also nice to have something simple enough that people could design a weight array without too much background knowledge. I think we can get the best of both worlds! :-) |
Yes, sounds good to me. As you mentioned the higher accuracy sum, worth mentioning this thread I started a while back. One of the interesting bits of it for me was learning that the default sum in Julia is pairwise, which should cut down floating point error a fair amount vs. a standard |
Thanks for sharing the link - interesting discussion. |
Adding the link to where the current implementation is documented https://juliarheology.github.io/RHEOS.jl/dev/numerical/ so any changes we make to the implementation we should try and update the docpage to reflect that |
Although I don't expect any speed changed will be significant compared to things like MittagLeffler evaluations, it would be interesting to benchmark the custom indexing pattern Vs. N floating point multiplications. I would guess the multiplications will be faster but would be nice to confirm this. Picking up something you mentioned in the opening comment:
If we do explore this option I think floating points will be best. The data type of the weights should be the same as the data type of the experimental data (RheoFloat) which they will multiply to avoid excess type casting. |
The current format of the weights array is not very intuitive, as pointed out by @fr293 . It is not an array of weights, and is difficult to use without the helper function.
Why not have a proper array of weights, integer or floating points? It should not be too difficult to implement, and probably more efficient. This line in particular reallocates a whole array for each evaluation of the cost function:
cost = sum((measured_weighted - convolved[weights]).^2)
A dot product of the weights vector and the squared difference would work well, possibly using a for loop so that no intermediate array is produced.
The text was updated successfully, but these errors were encountered: