Skip to content
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

Introduce support of subsolver records. #377

Merged
merged 21 commits into from
Apr 10, 2024
Merged

Conversation

kellertuer
Copy link
Member

@kellertuer kellertuer commented Apr 7, 2024

This is a PR to resolve #371.

  • generalise the record factories to work more modular (similar to debug), this allows to easier add elements to :Stop
  • add a RecordStoppingReason action to record the stopping reason (dah!)
  • add a RecordWhenActive (analogue to debug), though not 100% necessary, might be nice to reduce allocations when only requiring every $n$-th subsolver run to actually be recorded
  • add a RecordSubsolverRecords action that stores the sub solvers record (or parts of it) in the parents record entry for that iteration

ToDo

  • verify and check that the idea works
  • extend docuemntation
  • write tests
  • extend tutorial
  • check test coverage

Open Points

Currently the symbol :Subsolver is used in subsolvers to declare them beding dependent (of the parent), while it might be breaking :WhenActive might be a nicer symbol. The :Subsolver could be used to indicate subsolver recordings. I have to think about this a bit and whether this is seriously breaking, since for now :Subsolver. On the other hand it is not breaking code to the extend it does now work, just that since the last release to this then some might get a bit more subsolver debug.

@kellertuer kellertuer added enhancement WIP Work in Progress (for a pull request) labels Apr 7, 2024
…me `RecordSubsolverRecordings` to `RecordSubsolver` and use `:Subsolver` there instead (in recordings, not debug).
@kellertuer
Copy link
Member Author

The design is nice and seems to work in general. I went for :WhenActive that is much nicer to use in writing

The main thing that seems to bother a bit still is, that though there is a lot of resets implemented, it seems that the recording of subsolver recordings is still incremental, this needs to be fixed still.
So though recorded values are reset to the empty array the next push still pushes to the previous recordings array – no clue yet why.

@kellertuer
Copy link
Member Author

Got it to work, even with a small add-on such that with the keywords

        record = [ :Iteration, :Cost, (:Subsolver, :Stop)],
        sub_kwargs = [
            :debug => [:Stop, :WhenActive],
            :record => [:Stop => :Stop]
        ],

The subsolver records its stopping reason at the end, and the “Subsolver recorder” takes over everything recorded at the stop. (by default that second element is :Iteration, which takes over all recordings from there iterations of the subsolver).

One can of course also do :record => [:Stop => [:Stop, :Cost]], for the subsolver, than at every stop, the stopping criterion and the cost are recorded, and taken over by the subsolver recorder.

So with that this is feature complete (but not yet doc / test complete).

@kellertuer kellertuer removed the WIP Work in Progress (for a pull request) label Apr 8, 2024
@kellertuer kellertuer marked this pull request as ready for review April 8, 2024 13:01
Copy link

codecov bot commented Apr 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.76%. Comparing base (0d899bf) to head (610a873).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #377   +/-   ##
=======================================
  Coverage   99.76%   99.76%           
=======================================
  Files          74       74           
  Lines        7167     7241   +74     
=======================================
+ Hits         7150     7224   +74     
  Misses         17       17           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kellertuer kellertuer changed the title Initial sketch for proper support of subsolver records. Introduce support of subsolver records. Apr 9, 2024
@kellertuer kellertuer added the Ready-for-Review A label for pull requests that are feature-ready label Apr 9, 2024
@kellertuer kellertuer merged commit f546183 into master Apr 10, 2024
15 checks passed
@kellertuer kellertuer deleted the kellertuer/subsolver-record branch May 4, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Ready-for-Review A label for pull requests that are feature-ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a collection of records from a subsolver
1 participant