-
Notifications
You must be signed in to change notification settings - Fork 3
Stochastic transitions #539
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
base: main
Are you sure you want to change the base?
Changes from all commits
baeafb3
4037277
425223d
e3c6285
6cbcb6f
752154a
3965276
ac4e458
1e7c94f
930c10e
0b4e493
0e3bb45
224c2a6
211a7fd
98b7274
b955a10
3188662
a545715
61ffc2d
da06bbb
72f231d
76a1dba
1ee101c
efcf4c2
eb464d0
70f34cd
02c4cb9
269f712
c9dfa22
c6d990c
9dcb653
e39b780
8abb36a
2a8bc1e
3e76bf7
222ad88
c2fe4ae
d1c2223
c752086
59851ff
5e42910
86d19a6
aab2c7c
422ff3d
bedcfc8
b494a89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -448,7 +448,8 @@ | |
| raise Exception('Unknown TimeDependentConnections type - must be "transfer" or "interaction"') | ||
|
|
||
| def __repr__(self): | ||
| return '<TDC %s "%s">' % (self.type.title(), self.code_name) | ||
| # return '<TDC %s "%s">' % (self.type.title(), self.code_name) | ||
| return sc.prepr(self) | ||
|
|
||
| @classmethod | ||
| def from_tables(cls, tables: list, interaction_type): | ||
|
|
@@ -859,13 +860,13 @@ | |
| from_pop, to_pop = interaction | ||
| if not self.enable_diagonal and from_pop == to_pop: | ||
| raise Exception("Trying to write a diagonal entry to a table that is not allowed to contain diagonal terms") # This is because data loss will occur if the user adds entries on the diagonal, then writes the table, and then reads it back in | ||
| from_idx = self.from_pops.index(from_pop) | ||
| to_idx = self.to_pops.index(to_pop) | ||
| if boolean_choice: | ||
| value = "Y" if value else "N" | ||
| value = "Y" if (value.vals or value.assumption) else "N" | ||
| content[from_idx, to_idx] = value | ||
|
Comment on lines
865
to
867
|
||
|
|
||
| # Write the content | ||
|
Check failure on line 869 in atomica/excel.py
|
||
|
Comment on lines
863
to
869
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 In Extended reasoning...The bug: Where it manifests: Consider a transfer or interaction TimeSeries that has only a constant, explicitly-entered value of Why this causes data loss: The Why nothing else catches this: There is no validation elsewhere that cross-checks the boolean matrix against the underlying TimeSeries presence of data — the boolean cell is the sole source of truth for whether a row is included on read, so a truthiness bug here directly translates into silent data loss with no error or warning. Step-by-step proof:
Suggested fix: Replace the condition with a presence check, e.g. |
||
| for from_idx in range(0, len(self.from_pops)): | ||
| for to_idx in range(0, len(self.to_pops)): | ||
| row = start_row + 1 + from_idx | ||
|
|
||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1141,7 +1141,7 @@ | |
|
|
||
| return {(covout.par, covout.pop): covout.get_outcome(prop_coverage) for covout in self.covouts.values()} | ||
|
|
||
| def sample(self, constant: bool = True): | ||
| def sample(self, constant: bool = True, rng_sampler=None): | ||
| """ | ||
| Perturb programs based on uncertainties | ||
|
|
||
|
|
@@ -1158,9 +1158,9 @@ | |
|
|
||
| new = sc.dcp(self) | ||
| for prog in new.programs.values(): | ||
| prog.sample(constant) | ||
| prog.sample(constant, rng_sampler=rng_sampler) | ||
| for covout in new.covouts.values(): | ||
| covout.sample() | ||
| covout.sample(rng_sampler=rng_sampler) | ||
| return new | ||
|
|
||
|
|
||
|
|
@@ -1221,24 +1221,25 @@ | |
| """ | ||
| return "/year" not in self.unit_cost.units | ||
|
|
||
| def sample(self, constant: bool) -> None: | ||
| def sample(self, constant: bool, rng_sampler=None) -> None: | ||
| """ | ||
| Perturb program values based on uncertainties | ||
|
|
||
| Calling this function will perturb the original values based on their uncertainties. The | ||
| values will change in-place. Normally, this method would be called by | ||
| :meth:`ProgramSet.sample()` which will copy the ``Program`` instance first. | ||
|
|
||
| :param constant: If True, time series will be perturbed by a single constant offset. If False, | ||
| an different perturbation will be applied to each time specific value independently. | ||
| :param rng_sampler: Optional random number generator that may have been seeded to generate consistent results | ||
| """ | ||
|
|
||
| self.spend_data = self.spend_data.sample(constant) | ||
| self.unit_cost = self.unit_cost.sample(constant) | ||
| self.capacity_constraint = self.capacity_constraint.sample(constant) | ||
| self.capacity_constraint = self.capacity_constraint.sample(constant, rng_sampler) | ||
| self.saturation = self.saturation.sample(constant) | ||
| self.coverage = self.coverage.sample(constant) | ||
|
Comment on lines
1237
to
1241
|
||
|
|
||
|
Check failure on line 1242 in atomica/programs.py
|
||
|
Comment on lines
1231
to
1242
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 In Extended reasoning...The bug: self.spend_data = self.spend_data.sample(constant)
self.unit_cost = self.unit_cost.sample(constant)
self.capacity_constraint = self.capacity_constraint.sample(constant, rng_sampler)
self.saturation = self.saturation.sample(constant)
self.coverage = self.coverage.sample(constant)Why it manifests: Looking at Why nothing else catches this: Impact: For any program whose Step-by-step proof:
Fix: Pass self.spend_data = self.spend_data.sample(constant, rng_sampler)
self.unit_cost = self.unit_cost.sample(constant, rng_sampler)
self.capacity_constraint = self.capacity_constraint.sample(constant, rng_sampler)
self.saturation = self.saturation.sample(constant, rng_sampler)
self.coverage = self.coverage.sample(constant, rng_sampler) |
||
| def __repr__(self): | ||
| output = sc.prepr(self) | ||
| output += " Program name: %s\n" % self.name | ||
|
|
@@ -1397,25 +1398,28 @@ | |
|
|
||
| return len(self.progs) | ||
|
|
||
| def sample(self) -> None: | ||
| def sample(self, rng_sampler=None) -> None: | ||
| """ | ||
| Perturb the values entered in the databook | ||
|
|
||
| The :class:`Covout` instance is modified in-place. Note that the program outcomes are scalars | ||
| that do not vary over time - therefore, :meth:`Covout.sample()` does not have a ``constant`` | ||
| argument. | ||
|
|
||
| :param rng_sampler: Optional random number generator that may have been seeded to generate consistent results | ||
| """ | ||
|
|
||
| if self.sigma is None: | ||
| return | ||
|
|
||
| if rng_sampler is None: | ||
| rng_sampler = np.random | ||
|
|
||
| for k, v in self.progs.items(): | ||
| self.progs[k] = v + self.sigma * np.random.randn(1)[0] | ||
| self.progs[k] = v + self.sigma * rng_sampler.standard_normal(1)[0] | ||
| # Perturb the interactions | ||
| if self._interactions: | ||
| for k, v in self.interactions.items(): | ||
| self.interactions[k] = v + self.sigma * np.random.randn(1)[0] | ||
| self.interactions[k] = v + self.sigma * rng_sampler.standard_normal(1)[0] | ||
| tokens = ["%s=%.4f" % ("+".join(k), v) for k, v in self.interactions.items()] | ||
| self.imp_interaction = ",".join(tokens) | ||
|
|
||
|
|
||
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.
🟡 In
demo(), the fallback error for an unsupportedwhichreferences the undefined namedemosinstead ofdemo_projects(renamed by this PR). This causes aNameErrorto mask the intended informative exception whenever an invalid project name is passed todemo().Extended reasoning...
The bug: In
atomica/demos.py, this PR renames the localoptionslist to a module-leveldemo_projectslist (also newly exported via__all__). Theif which not in demo_projectscheck on line 51 correctly uses the new name, but the exception message on line 52 still refers to the old name via a variable calleddemos, which has never existed anywhere in the module:How it manifests: Python evaluates function arguments before the function call itself, so
"\n".join(demos)is evaluated first. Sincedemosis not defined in the local scope, module scope, or as a builtin/import, this raisesNameError: name 'demos' is not defined— and that NameError propagates instead of the intendedException("Supported demonstration projects are:...").Trigger path: Any call to
at.demo(which='invalid_name')(or anywhichnot present indemo_projects) hits this line. Nothing upstream validateswhichbefore this point — the default value was also changed fromNoneto'sir'in this PR, but that only affects the default argument; any explicit invalid string still reaches the broken exception line.Step-by-step proof:
at.demo(which='nonexistent').dtdict = sc.odict.fromkeys(demo_projects, 1.0)executes fine.if which not in demo_projects:evaluates toTruesince'nonexistent'isn't in the list.raise Exception(...)statement's arguments, starting with"\n".join(demos).demosfails (not a local, not a global indemos.py, not a builtin) →NameError: name 'demos' is not definedis raised instead of the intendedException.Why nothing prevents this: There's no test coverage for the invalid-
whicherror path (existing tests such astests/test_tox_library.pyonly exercise valid demo names viaat.demo_projects), so the mismatched rename slipped through.Impact: Limited to the error-reporting path for invalid input — a real exception is still raised (integration/valid-usage code paths are unaffected), but the message is a confusing
NameErrorrather than the helpful list of supported project names, which would be misleading for anyone debugging a typo'd demo name.Fix: Change
demostodemo_projectson that line: