Skip to content
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

sclang: print floats with at least 1 decimal place #3585

Merged
merged 2 commits into from
Mar 31, 2018
Merged

sclang: print floats with at least 1 decimal place #3585

merged 2 commits into from
Mar 31, 2018

Conversation

mossheim
Copy link
Contributor

Fixes #3573

Floats that are equal to an integer are printed with .0

I couldn't find a way to do this without actually scanning the string to
see if '.0' needs to be appended. "%.9f" will print extra significant
digits; and "%#.14g" will do the same. I think possibly switching on
the point where numbers change to scientific notation (1e14) and then
using "%#.14f" would work, but I'm not sure if that's a portable
solution and this seems a bit cleaner.


Before:

sc3> 3.0
-> 3

After:

sc3> 3.0
-> 3.0
sc3> 3.0000
-> 3.0
sc3> 3 - 0.0000000000000001
-> 3.0

@vivid-synth vivid-synth added the comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library" label Mar 18, 2018
@aspiers
Copy link
Contributor

aspiers commented Mar 18, 2018

Awesome, thanks!

@telephon
Copy link
Member

Ah good.

will inf return inf and not inf.0. And also the nan?

We'll be able to get rid of the code of storeOn in Float.sc:

storeOn { |stream|
		var	str;
		if(this == inf) { stream << "inf"; ^this };
		if(this == -inf) { stream << "-inf"; ^this };
		str = super.asString;
		stream << str;
		// if it doesn't already have a . or is 1e-05 then add a .0 to force it to Float
		if(str.find(".").isNil and: { str.find("e").isNil }) {
			stream << ".0";
		}
	}

actually, I just found a bug here:

(0/0).asCompileString // returns nan.0

@mossheim
Copy link
Contributor Author

@telephon - good catch! I've patched it and added tests, they pass locally.

Do you think it's ok for me to just remove Float.storeOn entirely and let it call SimpleNumber's? It appears that the current implementation has a similar bug anyway:

c = CollStream.new;
(0/0).storeOn(c);
c.contents; // "nan.0"

@mossheim
Copy link
Contributor Author

mossheim commented Mar 18, 2018

Also, I noticed this comment -

// the correct place to implement compileString for Float is in DumpParseNode.cpp
// int asCompileString(PyrSlot *slot, char *str)
// for now, solve it here.

Should I use this same method for compileString in the function mentioned? IIUC then I could get rid of Float.asCompileString's special implementation as well.

@telephon
Copy link
Member

The question is whether we want the storeOn and the printOn differ at all. If we make printOn print fewer digits, this makes large arrays more readable. But it might also be a little confusing.

If we take the two to be the same, I tend to recommend first to fix the storeOn method using your primitive call, and then implement printOn in terms of it. This makes clear that the number of digits posted are actually enough, and nobody has to wonder whether storeOn is insufficient.

@mossheim
Copy link
Contributor Author

The question is whether we want the storeOn and the printOn differ at all. If we make printOn print fewer digits, this makes large arrays more readable. But it might also be a little confusing.

I don't understand the difference between these two; I've never used them directly, and I can't find any documentation that explains it at any level of detail. It seems like printOn is to asString what storeOn is to asCompileString. If that is the case, then Float should have the same representation for both.

@telephon
Copy link
Member

I didn't realize that these two methods are undocumented. I'll try a short explanation.

Note that many implementations of these two methods could be still improved.

printOn (asString)

produce a string that represents the object close enough for a human reader. By default, this only returns the class name (e.g. a SinOsc), but usually is implemented in a way that the object can be recognized and distingushed (e.g. from other objects of the same class). Infinite collections (e.g. collections that contain themselves) are only partially printed, ending with ellipsis.

The REPL calls printOn (via asString) on the result when code is evaluated.

SinOsc.ar // -> "a SinOsc"

(freq: 600, amp: 0.1) // -> "( 'freq': 600, 'amp': 0.1 )"

Event.default // -> "(  )"

List[1, 2, 3] // -> "List[ 1, 2, 3 ]"

a = [1, 2, 3].add(a); -> "[ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2, 3, [ 1, 2...etc..."

storeOn (asCompileString)

produce a string that, when evaluated, returns an object equal to the object. For example the above Event.default.asCompileString will post the entire parent event.

This can be a way to store objects in a human readable form. Not all objects can be reproduced this way, and for some objects the representation is not possible or otherwise problematic. Then the method asTextArchive is used instead.

@telephon
Copy link
Member

telephon commented Mar 19, 2018

If that is the case, then Float should have the same representation for both.

Maybe yes. It would of course be ideal if, for printOn, 7/27 would return not 0.25925925925926 but 0.259… but that might be asked too much.

@aspiers
Copy link
Contributor

aspiers commented Mar 30, 2018

Thanks again for all your great work on this one folks! :-)

@mossheim
Copy link
Contributor Author

Thanks again for all your great work on this one folks! :-)

You're welcome!

Note that many implementations of these two methods could be still improved.

Could we do this in a separate PR @telephon ? This one is all ready to go as-is and I'm still not sure I understand what you want re: storeOn/printOn.

@telephon
Copy link
Member

@brianlheim sure!

@nhthn nhthn added this to the 3.10 milestone Mar 30, 2018
@mossheim mossheim merged commit 8c3563a into supercollider:develop Mar 31, 2018
@mossheim mossheim deleted the issue/3573 branch March 31, 2018 01:55
@jamshark70
Copy link
Contributor

I guess it's a bit of a side note, but this change accidentally broke Ambisonics Toolkit.

ATK looks up impulse responses based in part on the server's sample rate. The server reports the sample rate back to the client as a float. Then ATK does sampleRate = server.sampleRate.asString.

So what used to be a path blah/blah/44100/blah/blah is now blah/blah/44100.0/blah/blah and ATK reports that every possible sample rate is unsupported.

On the one hand, this change is technically sound. I'm not suggesting to revert.

On the other, something is wrong with our process if we all thought that string representations are purely cosmetic, and we all forgot that strings may refer to disk locations that are not flexible, and some parts of the path might be numeric. It bothers me that not a single one of us (myself, too) considered it at all.

@nhthn
Copy link
Contributor

nhthn commented Jun 5, 2018

yeah, it's definitely kinda troubling. i've documented it as a breaking change in the 3.10 changelog, so it will at least be very visible in the release notes.

@nhthn
Copy link
Contributor

nhthn commented Sep 12, 2018

dang. for the record, seems like we broke another thing: https://www.listarc.bham.ac.uk/lists/sc-users/msg62217.html

like i said in the thread, if your code breaks because of this change, it's probably relying on pretty fragile logic anyway. but either way, we have some lessons to take away from this. no one in particular is at fault, but we all majorly underestimated the impact of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants