search

dhoerl / EngineeringNotationFormatter

iOS Project demoing C-based Engineering Notation Formatter
10 stars 1 forks source link

Failing prefix conversions for small numbers #2

Closed martinmoene closed 11 years ago

martinmoene commented 11 years ago

Testing my EngFormat-Cpp - C++ based Engineering Notation Formatter variant, I found that one to three prefix results are wrong, e.g.:

Warning(119): Printing failing prefix tests:
'1.00 y' == '1000e-27'
'1.00 z' == '1000 y'

These failures occur at the lower end of the scale, also with EngNotation.c and depend on the compilers used.

ghost commented 11 years ago

Nice piece of work - I know C++ but no where as well as you! I downloaded your project and can get one error on a Mac, 64 bit, using llvm and its C++ libraries (which now are code complete for C++11. I'll try to figure out what the problem is. Reading the original code is a mind binder - it takes a bit to get into the swing of it.

Warning(119): Printing failing prefix tests: '1.00 z' == '1000 y' '1.00 a' == '1000 z' '1.00 n' == '1000 p'

martinmoene commented 11 years ago

There's a reason I didn't include a fix...

Acknowledgement: The test setup was gleaned from the implementation of std::optional by Andrzej Krzemieński.

ghost commented 11 years ago

I know what the problem is, its going to take me a bit of time to figure out how best to address it.

dhoerl commented 11 years ago

So I have a fix which is probably a wash in performance terms. I'm in the process of adapting your tests so I can use them too, in the Xcode project. I'll be doing a push request with the slightly modified test (basically to wrap them a bit more in macros, so changes by either of us can be cut and pasted. Just wanted you to know I'm on it as much as I can with my full time job.

Also, I'm going to drop the space that gets appended in the case where there is no prefix - it doesn't make any sense to add it, but I realize this is going to require you to tweak eng2exp, but I suppose you can just do one thing if there is no space. I see what you are trying to accomplish with only looking for one character after the space, but in the case of no SI postfix, then your code will grab the random character after any space separating the number from the next character (in say a sentence).

It would seem prudent to require the user to submit the actual eng string, with nothing appended, IMHO.

Other issues:

PS: with the new code I'm using this with no failures:

bool approx( double const a, double const b ) { double foo = a - b; return !isnormal(foo); }

martinmoene commented 11 years ago

Great! I'll have a look at it tomorrow.

dhoerl commented 11 years ago

I updated my post above a few times - can you comment on "the other issues" - do you care one way or the other? Nail those down and I can upload what I have.

I mean the two issues and the previous issue of "can I return a eng string with no appended space?

martinmoene commented 11 years ago

Drop the space that gets appended in the case where there is no prefix.

Agree: I had noticed the space, but did not act on that (yet).

Other issues:

  • what do you think about return "e0" instead of "" in the reverse prefix array?

Agree: I'm using eng_exponential for bool numeric in dbl2eng() indicating exponential notation: this change will let me keep that promise ;).

  • the new code is going to return a period in the engineering format regardless - so what was "900" before will be "900.", which I think is better (indicates a floating point number) - but I'm open to changing this to the old behavior

Neutral on this one. 123 kV, 123. kV reads as 122.5..123.5 kV either way.

PS: with the new code I'm using this with no failures:

WOW !

dhoerl commented 11 years ago

I uploaded the new code last night to github - I'm going to retrofit my fixes to your code as well shortly. You can see what I did in my version...

Really, I could not be happier that you are planning to use this. I did in the past and it was just great - having the step function. It occurred to me recently others might have a need, so I re-engaged with the code and did the iOS project. I had thought about making a C++ version, but never did.

I have two other projects that need C++ components - the Email Address verifier / searcher and the URL one, if you have nothing else to do :-)

dhoerl commented 11 years ago

I updated both this repository (now with unit tests based on your set along with several of my own), and also updated your C++ project.