Skip to content

fix: replace bare except with specific exceptions in solve_L, fix adj_cost docstring typo - #1091

Open
suyash469 wants to merge 6 commits into
PSLmodels:masterfrom
suyash469:fix/firm-bare-except-and-docstring
Open

fix: replace bare except with specific exceptions in solve_L, fix adj_cost docstring typo#1091
suyash469 wants to merge 6 commits into
PSLmodels:masterfrom
suyash469:fix/firm-bare-except-and-docstring

Conversation

@suyash469

Copy link
Copy Markdown

Summary

Two small real bug fixes in ogcore/firm.py.

Changes

1. solve_L() — Replace bare except: with except (ValueError, TypeError):

The try/except block exists solely to detect whether K_g is a NumPy array
(comparing an array to a scalar raises ValueError) or an incompatible type
(TypeError). A bare except: silently catches KeyboardInterrupt, SystemExit,
and MemoryError, making long OG-Core runs impossible to interrupt and masking
real errors silently.

2. adj_cost() — Fix docstring typo

'adjstment''adjustment'

Validation

  • Confirmed zero bare except clauses remain in firm.py via AST check
  • Typo verified absent from source

…_cost docstring typo

- firm.py solve_L(): Replace bare except: with except (ValueError, TypeError):.
  The try/except exists only to detect whether K_g is an ndarray (comparison
  raises ValueError) or an incompatible type (TypeError). A bare except catches
  KeyboardInterrupt, SystemExit, MemoryError and other critical signals, making
  long OG-Core runs impossible to interrupt and masking real bugs silently.

- firm.py adj_cost(): Fix typo in docstring: 'adjstment' -> 'adjustment'.
@jdebacker

Copy link
Copy Markdown
Member

@suyash469 Thanks for this PR!

I have following comments:

  1. Is there a way to streamline the new logic in firm.py? It seems to add a number of new lines to the module and only marginally increases the clarity of the code.
  2. Can you run make format from the top level directory to format the .py files?
  3. You might need some other edits to ensure tests pass.

@arihantlodha-cmd arihantlodha-cmd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR. The exception fixes are the right call: catching bare except: was genuinely hiding KeyboardInterrupt/SystemExit on long runs, and the specific exceptions match the intent (AttributeError for the delattr/setattr, (ValueError, TypeError) for the K_g array check in solve_L). I also like the np.any(x == 0) fix in get_MPx: the old np.any(x) == 0 was actually wrong (it asked whether "any element is truthy" equals 0, i.e. whether all elements are zero), so that's a real correctness fix.

I'm newer here so treat this as non-blocking, but a few things stood out.

The title undersells the PR. It reads as two small bug fixes, but the substantive change is vectorizing get_MPx and get_pm across industries and switching all the get_pm call sites to vectorized=True. That's a real change to a production-function hot path, not a cleanup, and someone reviewing on the title alone might not expect the pricing math to move. Might be worth splitting the exception fixes into their own tiny PR (fast to merge) and letting the vectorization stand on its own, or at least re-describing so the vectorization is front and center.

get_pm's SS return type changed. In the non-vectorized SS branch it now returns float(pmout[0]) instead of an array. The in-repo callers are all switched to vectorized=True so they're fine, but a downstream caller (a country model) doing get_pm(w, Y, L, p, "SS") would silently get a scalar instead of the array it used to get. Worth a changelog note at least, since it's a quiet interface change.

Is there a test that the vectorized path equals the old loop? The existing SS/TPI regression tests should catch a divergence if they run, but an explicit np.allclose(get_pm(..., vectorized=True), <per-industry loop>) would make the refactor easy to trust and guard it going forward.

Minor: get_MPx's default m changed from -1 to 0, which is fine since every caller passes m explicitly, but the docstring still says "when m=-1 the function computes it for the last industry," so the doc and the default disagree now.

Thanks for this, the interruptibility fix alone is worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants