-
-
Notifications
You must be signed in to change notification settings - Fork 93
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
Change default sig_figs value to 8 #1308
base: develop
Are you sure you want to change the base?
Conversation
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
also need to change the CmdStan docs |
Yes, will change several places if this gets merged |
whatever you and Bob think is best.
cheers,
Mitzi
…On Tue, Jan 14, 2025 at 8:42 PM Brian Ward ***@***.***> wrote:
also need to change the CmdStan docs
Yes, will change several places if this gets merged
—
Reply to this email directly, view it on GitHub
<#1308 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABJWT5CPNOZRIPOT7P5JWW32KW4IPAVCNFSM6AAAAABVFX4BO2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOJRGQ2TQMJTGE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Soliciting more opinions @bob-carpenter @SteveBronder @avehtari |
Can you try with the example in stan-dev/cmdstanr#420 (it's an issue in CmdStanR repo, but can be tested with any approach calling standalone generated quantities)? |
@avehtari for the specific example you link, it does appear to be resolved by this PR (passed over 100 times in a row) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a slipped default, so I took the liberty of suggesting a wording change while you're at it.
_default = "-1"; | ||
_default_value = -1; | ||
= "0 <= integer <= 18, or -1 to use the system library's default " | ||
"number of significant figures (usually 6)."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this 6
be an 8
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, -1 is preserved to do the same thing as before, which is to just not specify any target precision when the stream is created, leaving it with whatever your OS defaults to. This is why it’s the “system library’s default”, not ours — 6 comes from glibc and friends.
If we want, we can make -1 synonymous with our default (8) instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Though I should note, if you want to use Stan’s default, a reasonable option is just not pass this argument at all…)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a huge backward compatiblity concern and it would certainly be simple to have just one default. In which case, I don't think we'd need a behavior for -1, which would simplify the doc.
Having said that, I think it's OK for "unspecified" to be the Stan default and -1 the system default as long as it's a bit more clearly stated in the doc that the system default is the underlying system default, not the Stan default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to just not allow -1 at all, but removing a valid value would definitely break at least somebody’s code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point---I missed that was the old default!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the wording. Here's what the full help text reads as now:
sig_figs=<int>
The number of significant figures used for the output CSV files.
Valid values: 0 <= integer <= 18, or -1 to use the current operating system's default number of significant figures (This will vary, but on many systems it is 6).
Defaults to 8
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still find two things about this confusing:
- the use of "integer"---is that in other ones of these?
- there's no mention that the platform default is not the Stan default
But if you want to go with it like this, I'm just going to approve because it's not worth a lot of back and forth.
Submisison Checklist
./runCmdStanTests.py src/test
Summary:
Closes #1307
Intended Effect:
Increase the number of outputs saved by default
How to Verify:
Run a model without changing the sig_figs argument
Side Effects:
Additional storage may be required
Documentation:
TBD
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Simons Foundation
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: