-
Notifications
You must be signed in to change notification settings - Fork 468
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
m_as with a non-unit quantity #1415
Comments
When using Quantity with It is the expected behaviour, I've just realized this function is not present in the documentation. You are right about this. |
I on the other hand would expect |
Yes. As a side note, this is the reason why I opened this issue. I caught a bug that could have had catastrophical consequences in the real world. And avoiding catastrophes is the reason we used pint in the first place. |
I cannot agree with that affirmation. I'd rather say that it is producing a result that you did not expect but it is so because your expectation is "wrong" (in the sense that it is not the expectation with which it was implemented), but which is perfectly correct and physically meaningful according to its intended behaviour . IMHO, both expectations are equally reasonable, but the fact is that the one which was in the developer's mind was "give me the magnitude in the same units that this other quantity has". And I see no reason for changing the behaviour (which would introduce backwards-compatibility issues). So, I think that the current behavior should be correctly documented, also including a note to explicitly warn about this potential misunderstanding. At most, if the potential for confusion (even with a well-documented method) is deemed too high, I think that the |
As it is not documented, there is no definite "intended" behaviour. If at all, then it would be up to @hgrecco to decide. Even then, in the interest of continuous improvement, API's can change. If there is a behavioural change like this one - then it's usually done with a deprecation period and suitable warnings. But I repeat - as long as it wasn't documented, there was never a "public" API here.
Then please explain the physical meaning of the current behaviour. Given the length of a car Note that there is some asymmetry between The physical behaviour of Very different with You are concerned with breaking user code on relying on this behaviour? Take for example I do therefore believe that the current behaviour is at danger of silently creating wrong results whenever you cannot guarantee the unit of the argument. Because almost nothing in pint (or any physical quantity package for that matter) guarantees a specific unit beyond the physical meaning of a quantity, that means the only case where a breaking change would impact code that is not already broken is the case of a literal argument, e.g. |
IMHO, the key point of contention here is "whether the current behaviour should be considered API or not" My argument for that is that, in absence of explicit documentation, the current implementation is the source of truth. This of course is not a justification for all sorts of buggy behaviours, but in this case the implementation makes all the sense from a pragmatical programmatic point of view for concisely solving use cases such as "I've got two quantities, and I want the magnitude of the first one expressed in the same units as the second one". In other words, I want a concise way of expressing With that said, I think I've sufficiently expressed my point of view and that the opposing point of view is also sufficiently documented, so I will avoid feeding what to me starts resembling a bit like a flame war (it may be just my subjective impression, please do not take offence and accept my apologies if that is not the case). |
It is only a flame war, if we're pitting opinions against each other, rather than objective arguments. I was trying to make clear that there are objective arguments why I believe that behaviour that the current behaviour is effectively inconsistent. You bring forwards counter arguments - so I don't think this is a flame war. And I would like to counter your counter arguments 😉 as follows:
|
how about Also, to me the current behavior of |
It seems that I see the desire that |
+1 to deprecate passing a quantity to Regarding |
A bit of context here, a lot of methods in pint accepts This is perfectly valid and saying "this has no physical meaning" just does not make sense. It really is not an argument IMHO ❤️, we are a community of scientist, just because the current implementation does not suit you, saying that it's physically wrong does not make it True. Though I hear your arguments :), right now the API should be flexible enough to let you normalize quantities with another like @cpascual said with I personally had this use case where I needed magnitude normalized with a UnitLike '10^6 N' for computation. We can discuss a new method to normalize quantities with another while retaining dimension check as you said. That's something we can consider. I'm not in favour of changing the current behaviour with the scaled one. |
What I mean by non-physical is that neither the magnitude nor the unit are a property of a physical quantity (given a steel rod, you can measure its length but neither its magnitude nor its unit). They are a property of the representation of that quantity. Because of that, any computation whose result depends on the representation is necessarily not a model of a physical process, and thus a "wrong" result within a scientific computation. The same goes with the number type used to represent the magnitude. In the old days, rocket science would mean having a guidance system implemented using integer computation only. Any value that depends on a computation falling between two integer value would cause a "wrong" thrust output. A good engineer can deal with that - he chooses an implicit fixed-point representation with a suitable number of fractional digits. However, a lot of care has to be taken to always take note of the implicit exponent that is carried along with the computation. Things are much simpler with 64bit doubles, the workhorse of scientific computation. The processor automatically takes care of that exponent. A units library extends the same concept to the unit aspect of a physical computation. This leads to fewer errors, just like the use of doubles avoids most rounding errors you would get with integers. However, if I want to make use of that, I need to rely on the behaviour to be consistent. Otherwise, it instills distrust in the library. Consider what would happen if divisions with doubles would silently discard the fractional part of the dividend. If that were the case, one could just work around and be sure to never to do a division with non-integers and you are back to "mathematical" behaviour. Yet, I'm sure doubles would just be banned from all code-guidelines in fields with a heightened scrutiny. I accept that we are not going to change the return value of |
I agree with @jules-ch: we need some context to understand how we got here. Looking at the tracker, a large fraction of the recent issues have the same root: the creation of the In #227 it is clear that the case raised here was overlooked. The proponent was clearly thinking about a unit, not a quantity. @burnpanck and @lovasoa are right that this API might lead to unexpected results and silent bugs. We should fix it. But I also think that @cpascual is right: changing the behaviour will break a lot of code, deprecation of Quantity as an argument might be the best option. The question is how we move forward in a way that is consistent, surprise free and does not break peoples code.
Alternatively, we could split in two cases (but might be unnecesary complicated):
(For strings, i on the result after parsing.) In all cases provide a usefull message, could be something like this:
I am open to create another method for some of the use cases described here, but I would suggest we discuss this in another issue. |
I would expect
to either throw an error, or return
10
.However, it returns
100
, which is a little bit surprising (and unsafe).If m_as accepts a quantity as its unit parameter, than shouldn't it behave the same as if I had defined a new unit with my reference quantity and passed that to m_as ?
If the current behavior is actually intented, then maybe it could be highlighted in the documentation.
The text was updated successfully, but these errors were encountered: