map() function equation wrong #51
Comments
|
Hello @st42 , |
|
Not sure if the machine precision is really of influence here - the fact that this all uses (and rounds to) integers is. It's also a matter of intepretation. @st42 interprets map(x, 0, 3, 0, 1) as "distribute the integers {0, 1, 2, 3} over the integers {0, 1}. In this case, 0 and 1 should map to 0 and 2 and 3 should map to 1. However, that really only works when the input range is more than the output range. E.g. with @st42's proposal, you get the following: map(1, 0, 1, 0, 100) = 1 * 101 / 2 = 50 which seems weird. It seems that mapping the input max should at least give the output max (idem for min). This hints to an alternative intepretation: map(x, 0, 3, 0, 1) means to map the range [0, 3> to the range [0, 1> (here I mean range of real numbers in the mathematical sense). This means 0 maps to 0, 1 maps to 0.33, 2 maps to 0.67, 3 maps to 1 (note that the length of the ranges is 3 and 1, not 4 and 2 here!). Because the result is truncated to an integer, it ens up mapping 0, 1, and 2 to 0 and only 3 to 1. Perhaps the (implicit) truncated could be improved to do proper rounding, which would round 0.5 to 1 instead of 0, which is probably the proper solution for this problem? Also, as @agdl notes, the error is really only significant for small values, on larger values it's lost in the rounding anyway. |
Yes it is. This lead to another question: how to correctly round using only with integer arithmetic? |
|
Normal approach to rounding (for non-negative numbers) is to do 0.5 and then truncate. In the above equation, you could do this addition before the divide, so: which I think will get you as accurate as you can get. I think this even works for negative values, provided that the mapped values are inside the range given. Not sure how to handle negative values otherwise off-hand. |
|
To give a real world scenario as to why this needs changed consider this. You are using the arduino with a sensor to sample data and based on this data set a servo into 1 of 4 positions. The arduino uses a 10 bit adc and outputs integers from 0 to 1023. You would use map(x, 0, 1023, 1, 4). With the map function as it is currently the only time position 4 would be active is when the arduino outputs the integer 1023. With my proposed change each position would get an equal share of the possible range. When they converted this function from a float function to an integer function they didn't consider that the output is no longer a value but a position. Value mapping maps one value to another and the function works perfectly for floats. Positional mapping distributes the input as evenly as possible to the output and the function as is doesn't do that. This function needs to be split into two functions. One for floats as it is now and one for integers as I proposed. |
|
I agree that there are really two different kinds of maps and having two functions would be useful. I don't agree that the distinction here is "integers" vs "floats" - I might very well map 0,1023 to 0,5000 to get an ADC reading in millivolts. I need the "value mapping" as you say, but I don't have any need for floats. So I think think we should:
|
|
I would leave the current map implementation unchanged (too widely used, changing it is not backwards compatible) and add a new function that uses floats for better precision |
|
Please never use float. Thats totally inefficient. Edit: I calculated the equation myself (the whole thing, not only use the values) and i dont see the real problem. Normally it should just work fine. 1023*255/1023+0 = 255???
|
|
Can we close this? |
|
I guess so. @cmaglie ? |
|
No it can't be closed, there isn't still a good solution to this problem. Just to make it more clear, you can try the following test on your PC: #include <stdio.h>
// This is the original map() function in Arduino Core
long map(long x, long in_min, long in_max, long out_min, long out_max)
{
return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}
// This is the same but with the +1 "range extrension" as suggested by st42
long mapPlus1(long x, long in_min, long in_max, long out_min, long out_max)
{
return (x - in_min) * (out_max - out_min + 1) / (in_max - in_min + 1) + out_min;
}
// This is another version of map with rounding done only with integer calculations
// as suggested by M.Kooijman
long mapRound(long x, long in_min, long in_max, long out_min, long out_max)
{
return ((x - in_min) * (out_max - out_min) + (in_max - in_min)/2) / (in_max - in_min) + out_min;
}
int main(void) {
long x;
printf("Range 0-20 to 0-4\n");
printf(" x map map(+1) map(round)\n");
for (x=-10; x<=30; x++) {
printf("%6ld %6ld %6ld %6ld\n", x,
map(x, 0, 20, 0, 4),
mapPlus1(x, 0, 20, 0, 4),
mapRound(x, 0, 20, 0, 4));
}
printf("\n\n");
printf("Range 0-5 to 0-1023\n");
printf(" x map map(+1) map(round)\n");
for (x=-5; x<=10; x++) {
printf("%6ld %6ld %6ld %6ld\n", x,
map(x, 0, 5, 0, 1023),
mapPlus1(x, 0, 5, 0, 1023),
mapRound(x, 0, 5, 0, 1023));
}
return 0;
}The output is: The original map function gives good results on the extremes, but the distribution of numbers is not fair (for example -2 and 6 appears only at the extemes of the first table) The map function suggested by @st42 gives a better distribution but it's terrible when going from smaller to bigger ranges (see the second table), and it maps 30 to 7 where it should really be 6. The third version (by @matthijskooijman ) seems to handle better both extremes and distribution of number but has a strange behaviour with negative numbers (it seems to consider "0" and "-0" two different values). |
|
This should solve the issue: It handles both going from bigger to smaller ranges and smaller to bigger ranges |
|
The solution from @st42 works perfectly. Took me some time to understand the "why" and "how" but should be correct. Another question: should we also add a 2nd version that prevents wrong input? eg you input -10 but the input range is only 0-255. that the output is then automatically out_min Edit: long map(long x, long in_min, long in_max, long out_min, long out_max)
{
// if input is smaller/bigger than expected return the min/max out ranges value
if (x < in_min)
return out_min;
else if (x > in_max)
return out_max;
// map the input to the output range.
// round up if mapping bigger ranges to smaller ranges
else if ((in_max - in_min) > (out_max - out_min))
return (x - in_min) * (out_max - out_min + 1) / (in_max - in_min + 1) + out_min;
// round down if mapping smaller ranges to bigger ranges
else
return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
} |
|
I don't know if it's helpful, but this is how the ofMap() function works in openframeworks, a creative coding framework in c++: source code from here: https://github.com/openframeworks/libs/openFrameworks/math/ofMath.cpp |
|
It annoyed me that the out max and min could be surpassed long map2(long x, long in_min, long in_max, long out_min, long out_max)
{
auto res = (x - in_min) * (out_max - out_min + 1) / (in_max - in_min + 1) + out_min;
res = min(out_max, res);
res = max(out_min, res);
return res;
} |
|
I recently created an integer-only map() function which (I believe) solves all the problems @cmaglie mentioned above (over 5 years ago), including the "strange behaviour with negative numbers". It matches the results using floating point and with proper round off to the nearest integer. When used "backwards" it gives identical results (mapping 20-0 to 4-0 should be identical to mapping 0-20 to 0-4) and when used in "reverse" (mapping 0-20 to 0-4 compared with 0-20 to 4-0) gives the same set of results in reverse order. The only downside is the algorithm is quite complicated and slow.... long idealMap(long x, long in_min, long in_max, long out_min, long out_max) {
long in_range = in_max - in_min;
long out_range = out_max - out_min;
if (in_range == 0) return out_min + out_range / 2;
// compute the numerator
long num = (x - in_min) * out_range;
// before dividing, add extra for proper round off (towards zero)
if (out_range >= 0) {
num += in_range / 2;
} else {
num -= in_range / 2;
}
// divide by input range and add output offset to complete map() compute
long result = num / in_range + out_min;
// fix "a strange behaviour with negative numbers" (see ArduinoCore-API issue #51)
// this step can be deleted if you don't care about non-linear output
// behavior extrapolating slightly beyond the mapped input & output range
if (out_range >= 0) {
if (in_range * num < 0) return result - 1;
} else {
if (in_range * num >= 0) return result + 1;
}
return result;
}I also wrote of this on the Arduino forum. If anyone wants to compare them all, here's a sketch you can run on Arduino Uno or other boards to see them all side-by-side. const long in1 = 0;
const long in2 = 20;
const long out1 = 0;
const long out2 = 4;
void setup() {
Serial.begin(9600);
while (!Serial) ; // wait for Arduino Serial Monitor
delay(100);
Serial.println("map() function test");
printHeader(in1, in2, out1, out2);
for (long i = in1 - 10; i <= in2 + 10; i++) {
printLong(i, 4);
printLong(map(i, in1, in2, out1, out2), 7);
printLong(map(i, in2, in1, out2, out1), 5);
printLong(st42map(i, in1, in2, out1, out2), 7);
printLong(st42map(i, in2, in1, out2, out1), 5);
printLong(roundMap(i, in1, in2, out1, out2), 7);
printLong(roundMap(i, in2, in1, out2, out1), 5);
printLong(idealMap(i, in1, in2, out1, out2), 7);
printLong(idealMap(i, in2, in1, out2, out1), 5);
Serial.print(" ");
Serial.println(floatMap(i, in1, in2, out1, out2));
}
printHeader(in1, in2, out2, out1);
for (long i = in1 - 10; i <= in2 + 10; i++) {
printLong(i, 4);
printLong(map(i, in1, in2, out2, out1), 7);
printLong(map(i, in2, in1, out1, out2), 5);
printLong(st42map(i, in1, in2, out2, out1), 7);
printLong(st42map(i, in2, in1, out1, out2), 5);
printLong(roundMap(i, in1, in2, out2, out1), 7);
printLong(roundMap(i, in2, in1, out1, out2), 5);
printLong(idealMap(i, in1, in2, out2, out1), 7);
printLong(idealMap(i, in2, in1, out1, out2), 5);
Serial.print(" ");
Serial.println(floatMap(i, in1, in2, out2, out1));
}
}
float floatMap(float x, float in_min, float in_max, float out_min, float out_max) {
return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}
long idealMap(long x, long in_min, long in_max, long out_min, long out_max) {
long in_range = in_max - in_min;
long out_range = out_max - out_min;
if (in_range == 0) return out_min + out_range / 2;
// compute the numerator
long num = (x - in_min) * out_range;
// before dividing, add extra for proper round off (towards zero)
if (out_range >= 0) {
num += in_range / 2;
} else {
num -= in_range / 2;
}
// divide by input range and add output offset to complete map() compute
long result = num / in_range + out_min;
// fix "a strange behaviour with negative numbers" (see ArduinoCore-API issue #51)
// this step can be deleted if you don't care about non-linear output
// behavior extrapolating slightly beyond the mapped input & output range
if (out_range >= 0) {
if (in_range * num < 0) return result - 1;
} else {
if (in_range * num >= 0) return result + 1;
}
return result;
}
long st42map(long x, long in_min, long in_max, long out_min, long out_max) {
if ((in_max - in_min) > (out_max - out_min)) {
return (x - in_min) * (out_max - out_min + 1) / (in_max - in_min + 1) + out_min;
} else {
return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}
}
long roundMap(long x, long in_min, long in_max, long out_min, long out_max) {
return ((x - in_min) * (out_max - out_min) + (in_max - in_min) / 2) / (in_max - in_min) + out_min;
}
void loop() {
}
void printLong(long n, int nchar) {
char format[6], buf[12];
sprintf(format, "%%%dd", nchar);
sprintf(buf, format, n);
Serial.print(buf);
}
void printHeader(long in_min, long in_max, long out_min, long out_max) {
Serial.println();
Serial.print("Testing map ");
Serial.print(in_min);
Serial.print(" - ");
Serial.print(in_max);
Serial.print(" ----> ");
Serial.print(out_min);
Serial.print(" - ");
Serial.println(out_max);
Serial.println();
Serial.println("Input ArduinoMap st42map roundMap idealMap floatMap");
} |
|
Here is one I did and proposed MANY years ago, when there was a long heated discussion on this topic. |
|
Here is a test sketch that can be run on an Arduino to test range sets.
To use, modify the variables: Then compile and upload to your Arduino and look at the serial port output.
here is the code: #if 0
// this is the map() function that is currently in the Arduino.cc core library
long map(long x, long in_min, long in_max, long out_min, long out_max)
{
return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}
#endif
// a "rounding" version that some suggest
long maprnd(long x, long in_min, long in_max, long out_min, long out_max)
{
return (x - in_min) * (out_max - out_min+1) / (in_max - in_min+1) + out_min;
}
// this is the map() function that is currently in the ESP866 platform core library
long mapESP(long x, long in_min, long in_max, long out_min, long out_max) {
const long dividend = out_max - out_min;
const long divisor = in_max - in_min;
const long delta = x - in_min;
return (delta * dividend + (divisor / 2)) / divisor + out_min;
}
// bperybap version
long newmap(long x, long in_min, long in_max, long out_min, long out_max)
{
if( x == in_max)
return out_max;
else if(out_min < out_max)
return (x - in_min) * (out_max - out_min+1) / (in_max - in_min) + out_min;
else
return (x - in_min) * (out_max - out_min-1) / (in_max - in_min) + out_min;
}
long fromLow = 0;
long fromHigh = 10;
long toLow = 10;
long toHigh = 0;
void setup(void)
{
long val, mval, mval2, mvalESP, mvalnew;
long dir = 0;
char buf[128];
Serial.begin(115200);
while(!Serial){}
delay(500);
Serial.print("\n\n");
if(fromLow < fromHigh)
dir = 1;
else
dir = -1;
for( val = fromLow;; val += dir)
{
mval = map(val,fromLow,fromHigh, toLow,toHigh);
mval2 = maprnd(val,fromLow,fromHigh, toLow,toHigh);
mvalESP = mapESP(val,fromLow,fromHigh, toLow,toHigh);
mvalnew = newmap(val,fromLow,fromHigh, toLow,toHigh);
sprintf(buf, "map(%3ld,%3ld,%3ld,%3ld,%3ld)",
val, fromLow, fromHigh, toLow, toHigh);
Serial.print(buf);
sprintf(buf, " map:%3ld, maprnd:%3ld, mapESP:%3ld, newmap:%3ld\n",
mval, mval2, mvalESP, mvalnew);
Serial.print(buf);
if(val == fromHigh)
break;
}
}
void loop(void){} |
|
There is additional discussion on the subject at arduino/Arduino#10382, which I am closing as a duplicate of this one. |
|
So why can't we fix this with either my proposed map function or Paul's templates as either do the mappings correctly (the way people would expect) and work for some cases where others fall flat on their face ad screw up for certain ranges like the current Arduino.cc IDE supplied map() function, the map() function proposed in the first post in this issue, and the ESP8266 core map() function. IMO, I think Paul's template is the way to go as it works properly and as expected for more than just ints. How big of deal is it that fixing the existing map() function with better code that works more correctly potentially breaks some small amount of existing code that has attempted to work around the issue by fudging up the parameters? It seems like we have a working solution for the calculation and it is now a question of deciding on the details of how to get it integrated. The reason I bring up a math or map object is that this math object could be used for all kinds of Arduino math functions that also have issues. Seems like once we decide these questions, a solution could be integrated into the IDE fairly easily / quickly. |
|
I'm pretty sure the answer to "why can't we" comes down to Arduino's priorities as a company. |
EDIT NOTE: I added a comment to the end of this from a later comment because I thought it should have been included in the first place.
The equation in the function stated at http://arduino.cc/en/Reference/Map
is wrong. The example given:
In this example 0-1023 is mapped to 0-255. The equation gets it wrong by mapping 1023 numbers to 255 numbers, when it should be mapping 1024 numbers to 256 numbers.
0 through 1023 is 1024 numbers not 1023 : in_max - in_min : 1023-0=1023
0 through 255 is 256 numbers not 255 : out_max - out_min : 255-0=255
The corrected equation in the function should be:
In the following simpler example it will be easier to see:
val = map(val, 0, 3, 0, 1);
In this example 0,1,2 and 3 is mapped to 0 and 1. Mapped correctly it should be:
0 maps to 0
1 maps to 0
2 maps to 1
3 maps to 1
Using the equation in the the function as it is now, it is mapped as such:
0 maps to 0
1 maps to 0
2 maps to 0
3 maps to 1
This is a common problem programmers have dealt with before. When dealing with arrays you can not determine array size by simply subtracting first position from last position you will always be off by 1. That is why you add 1 to get proper size. I know this is for the most part a minor problem, but if your project requires accurate results this could be a serious issue.
--- Added from later comment---
To give a real world scenario as to why this needs changed consider this. You are using the arduino with a sensor to sample data and based on this data set a servo into 1 of 4 positions. The arduino uses a 10 bit adc and outputs integers from 0 to 1023. You would use map(x, 0, 1023, 1, 4). With the map function as it is currently the only time position 4 would be active is when the arduino outputs the integer 1023. With my proposed change each position would get an equal share of the possible range. When they converted this function from a float function to an integer function they didn't consider that the output is no longer a value but a position. Value mapping maps one value to another and the function works perfectly for floats. Positional mapping distributes the input as evenly as possible to the output and the function as is doesn't do that.
This function needs to be split into two functions. One for floats as it is now and one for integers as I proposed.
The text was updated successfully, but these errors were encountered: