fix(RVFWHMmodel): preserve prior precision in save_setup#57
Open
dgegen wants to merge 2 commits intokima-org:mainfrom
Open
fix(RVFWHMmodel): preserve prior precision in save_setup#57dgegen wants to merge 2 commits intokima-org:mainfrom
RVFWHMmodel): preserve prior precision in save_setup#57dgegen wants to merge 2 commits intokima-org:mainfrom
Conversation
…ision Without std::fixed, the default stream formatting uses 6 significant figures, which truncates large BJD-scale values (e.g. Tcprior bounds ~2458520.644 and ~2458520.656 both round to 2.45852e+06). This caused load_results to fail with "Uniform distribution must have lower < upper limits". Also removes the narrow precision(15)/precision(6) workaround around M0_epoch, which was a symptom of the same root cause.
There was a problem hiding this comment.
Pull request overview
This PR fixes precision loss when RVFWHMmodel::save_setup serializes model priors to kima_model_setup.txt, preventing large-magnitude prior bounds (e.g., BJD-scale times) from being rounded into identical values and subsequently breaking load_results.
Changes:
- Enable
std::fixedformatting for the setup-file stream inRVFWHMmodel::save_setup(matchingRVmodelbehavior). - Remove the local
precision(15)/precision(6)formatting toggle aroundM0_epoch.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…across all models Add `std::fixed` and `fout.precision(15)` globally in `save_setup` for all models that were missing it (BINARIESmodel, ETVmodel, GAIAmodel, GPmodel, OutlierRVmodel, RVFWHMRHKmodel, RVGAIAmodel, SPLEAFmodel, TRANSITmodel) and add `fout.precision(15)` to models that already had `std::fixed` but lacked explicit precision (ApodizedRVmodel, RVFWHMmodel, RVHGPMmodel, RVmodel). Also removes the narrow `precision(15)`/`precision(6)` workarounds around `M0_epoch` that were present in several files, as these are now superseded by the global setting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
RVFWHMmodel::save_setupwrites the setup file withoutstd::fixed, so the default C++ stream formatting uses only 6 significant figures. For large BJD-scale values, this truncates distinct prior bounds to the same value — e.g.Tcpriorbounds2458520.644883and2458520.656756both become2.45852e+06. Whenload_resultsreads the setup file back, it fails with:This affects any prior whose bounds are large-magnitude values, and is likely the root cause of the error reported in #24.
RVmodelalready usesstd::fixedand is not affected.Fix
std::fixedtofoutat the start ofsave_setup, matching the approach inRVmodelfout.precision(15)/fout.precision(6)workaround aroundM0_epoch, which was a narrow patch for the same root causeAfter applying this fix and recompiling,
RVFWHMmodelruns as before, butload_resultsnow correctly reads back the setup file for models with the correct large-magnitude prior bounds for BJD-scale transit times, allowing me to use this model.Note
The same pattern existed in
BINARIESmodel,ETVmodel,GAIAmodel,GPmodel,RVFWHMRHKmodel,RVGAIAmodel,SPLEAFmodel, andTRANSITmodel. I have applied the same fix to all of these, and additionally addedfout.precision(15)globally across all models to ensure full double precision is preserved regardless of the magnitude of prior bounds.Let me know what you think!
Cheers,
David