Replies: 2 comments
-
|
Thanks for the report. |
Beta Was this translation helpful? Give feedback.
0 replies
-
|
Thanks for the quick reply and update! |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
First of all, thank you for maintaining biomass and the sample models.
Summary
init_XXXappear alongside parameters and can be confused with constants (e.g.,C.init_EGFRinpan_rtk).to_csv()allocates withdtype='<U21'→ long labels get truncated.savefig()reuses an existing CSV → plots can be stale unlessto_csv()is called first.Details (env, context, repro, proposals)
Module:
biomass.result.OptimizationResultsVersion:
biomass==0.14.0Environment: Python 3.12, OS macOS, matplotlib 3.10.5, seaborn 0.13.2
Context
While visualizing estimated parameter sets with
OptimizationResults, I ran into two classes of issues:UX/consistency
to_csv()writes initial values with theinit_prefix (e.g.,init_EGFR) and parameters side-by-side in the same CSV.savefig(), initials appear alongside parameters and can be mistaken for constants defined in the base model (e.g., parameter nameC.init_EGFRvs initial-value labelinit_EGFR). They are conceptually different but look very similar in the figure.pan_rtk(https://github.com/biomass-dev/biomass/tree/master/biomass/models/pan_rtk) includes a constant/parameter namedC.init_EGFR, which can easily be confused withinit_EGFRproduced byto_csv()for the species initial value.Technical
to_csv()allocates the CSV buffer withdtype='<U21', so long labels are truncated (includinginit_...names).savefig()reuses an existing CSV if present. If the model or indices changed, it’s easy to get stale plots unless users remember to callto_csv()manually.to_csv()docstring mentions two files:but the current implementation writes a single CSV mixing both.
None of these are critical bugs, but together they create confusion and friction in day-to-day use.
Minimal Repro
C.init_EGFRand also an initial value for speciesEGFR(soidx_initialscontains that species index); for example, the pan_rtk sample model:What I would expect
Proposed (backward-compatible) changes
to_csv()Change the buffer dtype to
object:This prevents cutting long names and keeps existing behavior otherwise intact.
savefigoptionsinclude_initials: bool = False(default False)→ filter out rows starting with
init_unless the user explicitly opts in.pretty_labels: bool = True(default True)→ when initials are included, render
init_XXXasInit: XXXto reduce confusion with parameters likeC.init_XXX.refresh: bool = True(orFalseif you prefer strict backward compatibility)→ when
True, callto_csv()before plotting to avoid stale outputs.Sketch:
optimized_params.csvandoptimized_initials.csv, orinit_prefix plus the newsavefigoptions.Questions for maintainers (seeking guidance)
init_XXX?to_csv()before plotting?Why this might help
C.init_EGFR) and initial values (init_EGFR).(Optional) Compatibility notes
dtype=objectinto_csv()prevents truncation without changing CSV semantics.savefigoptions could be opt-in, preserving current behavior unless explicitly enabled.init_prefix) and CSV refresh would likely suffice.Note: This write-up was drafted with assistance from ChatGPT; I reviewed and validated all observations and code snippets. Thanks again for maintaining biomass and the sample models.
Beta Was this translation helpful? Give feedback.
All reactions