Introduce get_counters accessor - #562
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a get_counters accessor and updates the NLP/NLS counter plumbing (including display and the @default_counters macro) to use it, reducing direct coupling to a counters field name and enabling alternative counter storage layouts in concrete models.
Changes:
- Add and export
get_counters(::AbstractNLPModel)(defaulting tonlp.counters) and route counter access through it. - Update NLP/NLS
show,sum_counters, and counter getter/increment/reset logic to useget_counters. - Update tests and guidelines to reference the new accessor.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/nlp/utils.jl | Updates wrapper-model counter test to compare via get_counters instead of direct field access. |
| src/nls/show.jl | Uses get_counters(nls) when displaying NLS counters. |
| src/nls/counters.jl | Routes NLS counter access/increment/reset/sum through get_counters. |
| src/nlp/utils.jl | Updates @default_counters forwarding of .counters to use get_counters(inner). |
| src/nlp/show.jl | Uses get_counters(nlp) when displaying NLP counters. |
| src/nlp/counters.jl | Exports/defines get_counters and updates counter APIs to call it. |
| docs/src/guidelines.md | Updates show example to use get_counters, plus formatting tweaks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Furthermore, we define a general `show` that calls `show_header` and specific `show` functions for the `meta` and the `counters`. If your model does not have `counters` in the default location, you must define `show` for them as well. Alternatively, you may desire to change the behaviour of show. Here is an example, again from `SlackModel`: | ||
|
|
There was a problem hiding this comment.
This section says models without counters in the default location "must define show for them", but the codebase now uses get_counters(nlp) in the default show. With this PR, overriding get_counters should generally be sufficient for show to work without custom show methods; the guideline text should be updated accordingly.
There was a problem hiding this comment.
First, I need to implement get_counters for SlackModels.
| If a model does not implement `counters`, then it needs to define | ||
|
|
||
| - `neval_xxx(nlp)` - get field `xxx` of `Counters` | ||
| - `reset!(nlp)` - resetting all counters | ||
| - `increment!(nlp, s)` - increment counter `s` |
There was a problem hiding this comment.
The "Advanced counters" guidance is now misleading: a model can store counters under a different field name and just implement get_counters(nlp) to reuse the existing neval_*, increment!, reset!, etc. implementations. Consider updating this section to recommend overriding get_counters first, and only implementing all neval_xxx/reset!/increment! manually when the counters are not representable as a Counters object.
|
6d1c37b to
a4b6102
Compare
An accessor is more robust than accessing to an attribute explicitly, and allows concrete models to have Counters attributes that are not named `counters`. For backward compatibility, get_counters(model) = model.counters, though that may change in the future.
An accessor is more robust than accessing to an attribute explicitly, and allows concrete models to have Counters attributes that are not named
counters.For backward compatibility, get_counters(model) = model.counters, though that may change in the future.