deprecate derivatives and error_components #337
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #337 +/- ##
==========================================
+ Coverage 87.15% 87.17% +0.01%
==========================================
Files 20 20
Lines 2017 2019 +2
==========================================
+ Hits 1758 1760 +2
Misses 259 259
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #337 will not alter performanceComparing Summary
|
|
This PR is ready for review |
|
@newville @wshanks @andrewgsavage this small PR is ready for review. These two attributes are being either removed or changed in 4.0 so we need to mark them as deprecated in a 3.x release. |
|
@newville @wshanks @andrewgsavage bump again, looking for feedback on the wording. If I don't hear anything in a week I'll probably move towards merging it myself. |
| This mapping is cached, for subsequent calls. | ||
| """ | ||
| warn( | ||
| f"{self.__class__.__name__}.derivatives() is deprecated. It will " |
There was a problem hiding this comment.
In #339 I already asked if it had already been settled to change error_components instead of use a different name for the property and keep the method. I was also wondering about derivatives. Is it correct to summarize that the old code kept both the derivative and the weight of the uncertainty atom as separate values and multiplied them together to get the final uncertainty while the new code eagerly multiplies them and just keeps weights? What is the trade off there? Is the decision that keeping only the final weights makes the code simpler and the derivative information is not interesting enough to keep?
There was a problem hiding this comment.
Let's see if I can explain this difference. This is sort of the critical architecture difference so I should be able to.
Is it correct to summarize that the old code kept both the derivative and the weight of the uncertainty atom as separate values and multiplied them together to get the final uncertainty while the new code eagerly multiplies them and just keeps weights?
The AffineScalarFunc object stored a mapping from Variable constituents to the derivative of the function with respect to that Variable. When requested, e.g. during std_dev calculation, the error_components were calculated by multiplying the std_dev of the Variable by its corresponding derivative. I think lebigot had the function f(x, y) = f(x0, y0) + df/dx dx + df/dy dy in mind.
In the new framework we wanted to do away with the Variable/AffineScalarFunc distinction. Now we have UAtom which doesn't inherit from UFloat. In this case, for each UFloat, we keep track of a mapping from UAtom to a float weight. The std_dev of a UAtom is always unity, so we don't need to keep track of it. But if one UAtom moves through multiple operations we don't keep track of the intermediate derivates. The equation we have in mind is something like f(x, y) = f(x0, y0) + wa da + wb db + wc db + ... where da, db, dc are UAtom on which x and y random variables my covariantly depend. We use derivatives to calculate updated UCombo linear combination coefficients as as we do computations, but these derivative calculations are considered intermediate results which are NOT saved.
So what you say is close to correct but not quite. The old code stores the derivatives and uses the derivatives and std_devs lazily when calculating composite std_dev. The new code doesn't store std_devs of UAtom's because they're always unity. It eagerly multiplies derivatives into the new linear combination. The driver for all of this is a move away from the idea of AffineScalarFunc being a function and moving towards the idea of UFloat being a random variable.
What is the trade off there? Is the decision that keeping only the final weights makes the code simpler and the derivative information is not interesting enough to keep?
What you say in the second sentence is essentially what I think. I think the random variable approach makes the code simpler. Getting rid of the derivatives concept allows us to eliminate the AffineScalarFunc/Variable confusion. The new framework doesn't even have a way that we could store these derivatives if we wanted to because we don't keep track of the "history" of a random variable. To do so you need a privileged class of Variable objects with respect to which derivatives can be taken and that is exactly what we were trying to eliminate. At #251 (comment) I give some more explication about what I think are the downsides of the derivatives framework.
And then yes, I think the derivative information is not interesting to keep. I don't think uncertainties should try to make derivatives computation a selling point. numdifftools or sympy and others exist for that.
I'll address your error_components question in the other thread where I'm more open to a shim, but it is actually very important and not really optional for this refactor that we do get rid of UFloat.derivatives.
There was a problem hiding this comment.
I don't use .derivatives myself so I am not that attached to it. I do see some usage searching GitHub though. So it is hard for me to assess how important that feature is to users.
I think AffineScalarFunc vs Variable issue is separate from whether derivatives can be kept. Having AffineScalarFunc or Variable inherit from the other and having both be user facing depending on what operations have been performed on an object with uncertainty I agree are not a good ways for things to operate for users or developers. I like the way #262 makes UFloat (replacing AffineScalarFunc) the only public facing uncertain object and makes UAtom (replacing Variable) completely hidden.
I think though that the system in #262 could work similarly with UAtom having a std_dev attribute that held that uncertain variable's weight of uncertainty and with UFloat creating a UCombo with that UAtom with a weight of 1 in the UCombo. In this case, the UCombo weight is the derivative. The propagation would work similarly but just the weights would be propagating the derivatives and to get the UFloat overall standard deviation would require multiplying the UCombo weights by the UAtom weights before summing. So it doesn't seem that different to me. I don't love the idea of mutating the std_dev of a created value with uncertainty but, since equality is based on UFloat nominal value and the weights and uuids of UAtoms, I don't think it actually breaks anything related to hashing or equality to do it (the action at a distance concern).
There was a problem hiding this comment.
@wshanks ok thanks, this is a helpful comment and I'm excited to have this discussion because it's very important I think. I forgot the details. Yes, I agree with you that the need to drop derivatives comes more from the idea that UAtom should have unity, rather than controllable, std_dev rather than the AffineScalarFunc/Variable issue.
So the downsides to letting UAtom have non-unity std_dev are:
- More complicated data model.
- More literal data
- More complicated calculations. We're already having a performance regression, see Refactor Speed Benchmarking #327. We would want to ensure added computation from non-unity
UAtomstd_devdon't appreciably impact performance.
I don't love the idea of mutating the std_dev of a created value with uncertainty...
Which object would have a mutating std_dev? I think UAtom would not have mutating std_dev under your proposal. UCombo already has a _std_dev attribute that mutates from "unset" to it's value the first time it is calculated. I think it would be the same under your proposal.
Another downside of the derivative concept is that there is a question: with respect to what are we taking the derivative. In master the answer is that we are taking the derivative of the special uncertain objects that were instantiated using the ufloat, correlated_values, or correlated_values_norm functions. Under your proposal is that we are taking derivatives with respect to the UAtom objects that get instantiated during ufloat, correlated_values, or correlated_values_norm. But that gives a special privilege to certain uncertain numbers that I don't think is warranted. For example, I can do
x = UFloat(1, 0.1)
y = 2 * x
z_x = sin(x)
z_y = sin(y)
Under your proposal, I can access the derivative of z_x with respect to the single UAtom that goes into x and get the answer I'm expecting. This is kind of like the derivative of z_x with respect to x. I can also access the derivative of z_y with respect to the single UAtom that goes into x and get the answer I'm expecting. But your proposal (and the code on master) doesn't provide a way to access anything like the derivative of either z_y or z_x with respect to y. You could look at the derivatives of both z_y and y with respect to the UAtom that goes into x and do some math on your end to squeeze something out, but this isn't really what you want.
I think when you shift from thinking of UFloat as a function (AffineScalarFunc) and move to thinking about it as a random variable, the importance of derivatives goes away. What exists, rather, is random variables and correlations between those random variables. I think the current code on #262 is an architecture that minimally accomplishes this data model.
One final comment
I like the way #262 makes UFloat (replacing AffineScalarFunc) the only public facing uncertain object and makes UAtom (replacing Variable) completely hidden.
UAtom is not "completely" hidden. When a user does UFloat.error_components they get a dictionary whose keys are UAtom instances. I also imagine a json serialization algorithm that uses the UAtom objects and their uids to generate a human readable serialization of a UFloat object. In my opinion, this means that UAtom is part of the public API and needs to be documented. There is an argument to be made that we could ALSO eliminate the error_components property/function. Then UAtom would be truly hidden and the case for eliminating derivatives would be closed. I'm not totally opposed to this idea since I find the use cases for error_components a little strange, especially in light of the re-architecture. In this case users would shift to calculating correlations and covariances between different UFloat obects to figure out how much the uncertainty in x contributes to the uncertainty in y. This is the definition of covariance.
There was a problem hiding this comment.
@wshanks curious for your thoughts on deprecating UFloat.derivatives. I think it has a poor data model that is very far removed for the 4.x data model of UFloat as random variable. The issue is that the UFloat.derivatives attribute doesn't make sense for any combination of two UFloat instances. In uncertainties < 4.0 UFloat.derivatives at least related UFloat instances but the relation wasn't symmetric. You could calculated dy/dx, but not dx/dy.
In uncertainties > 4.0 it would relate UFloat instances to UAtom instances which is even more awkward.
I think the symmetric relationship between variables that we really want is covariance or correlation as I bring up in #345.
@wshanks Can you share your methodology for finding usages of UFloat.derivatives on github and share some of the examples you found? I would be curious to look at them.
There was a problem hiding this comment.
Which object would have a mutating std_dev? I think UAtom would not have mutating std_dev under your proposal. UCombo already has a _std_dev attribute that mutates from "unset" to it's value the first time it is calculated. I think it would be the same under your proposal.
Right, I just wanted to respond to newville's comment here about the action at a distance being more of a wart than a feature. When you keep the standard deviations inside the Variable/UAtom, you leave open the ability for the user to mutate UAtom.std_dev and change the error of downstream UFloats. I just wanted to say that I still would not want that to be a supported feature but the user could do that if they wanted.
that gives a special privilege to certain uncertain numbers that I don't think is warranted.
Yes, I do think of the Variable/UAtom objects as privileged. I think this relates back to your previous comment:
I don't think uncertainties should try to make derivatives computation a selling point.
I don't think Uncertainties needs to be a tool for tracking derivatives of all computations, but the derivatives relative to the underlying independent variables do make sense to track to me because fundamentally we have these random variables of some size and then weights for how much those different random variables contribute to downstream calculations. Maybe derivatives is too generic a term because really what is is the weights of the independent random variables.
UAtom is not "completely" hidden....
Right. What I meant was that when the user uses ufloat to generate initial values with uncertainty they get objects that are the same type as all further calculated results instead of needing to care that at first they get Variable and then later those transform into UFloat when they do something like multiply by 2. The rest of your point here is similar to the way the discussion is going in #336. We could decide that tracking the underlying independent variables is not interesting enough and the user just needs to use covariance methods to find those weights themselves if they want them.
curious for your thoughts on deprecating UFloat.derivatives
Can you share your methodology for finding usages of UFloat.derivatives on github and share some of the examples you found? I would be curious to look at them.
I gave a search string in #336 and commented some there. I actually could not find non-trivial usage of .derivatives, only error_components.
I think I am coming around on the idea of dropping derivatives and error_components. If we did that, would we also deprecate tag? It is only useful for labeling the UAtoms which would then be hidden. On the flip side, I could see keeping #262 working the way it does but tagging the std_dev onto the UAtom objects. Then it would be there in case a user did want to work out the weights of all the UAtoms like derivatives provides, but we might decide that that is niche enough that the user just needs to keep track of the ufloats corresponding to independent variables themself.
There was a problem hiding this comment.
@wshanks You have lots of good interesting responses I want to get back to between this thread and the other one but my time is short. I want to just give a quick response now
I think I am coming around on the idea of dropping derivatives and error_components. If we did that, would we also deprecate tag? It is only useful for labeling the UAtoms which would then be hidden. On the flip side, I could see keeping #262 working the way it does but tagging the std_dev onto the UAtom objects. Then it would be there in case a user did want to work out the weights of all the UAtoms like derivatives provides, but we might decide that that is niche enough that the user just needs to keep track of the ufloats corresponding to independent variables themself.
I feel strongly about dropping UFloat.derivatives, I will explain in more detail as I have time. I would prefer to drop UFloat.error_components and UAtom.tag but, as you point out, there is a use case involving tags and error_components that we would then lose. This use case is outlined in the documentation. Again, as you point out, the user could keep track of this on their own and calculate covariances. I think we should provide a code snippet how the transition from error_components to covariance would look. Your research revealed that scinum is a serious library using this functionality. We could reach out to them directly to try to get their thoughts on this change.
I think that if we drop error_components we should also drop tags.
|
@jagerber48 sorry for the delay, this has a been a very busy time for me. I think this is fine, and agree the focus here should emphasize uncertainty propagation, and that the new design with In fact, I think the "implied function/property" or "action at a distance" of: in which |
Yeah, this behavior is highly undesirable. Another issue is that you can set |
…s_and_error_components
3 participants
pre-commit run --all-fileswith no errors.
AffineScalarFunc.derivativespropert as deprecatedAffineScalarFunc.error_components()as deprecated and that it will be replaced with a propertyAffineScalarFunc.error_components.