Skip to content

Conversation

@casperdcl
Copy link
Member

@casperdcl casperdcl commented Feb 2, 2026

Follow-up to #1659; related to PETRIC2

  • allow update_objective_interval=np.inf
  • allow update_objective_interval=0
  • For debate: deprecate Algorithm(update_objective_interval) in favour of Algorithm.run(update_objective_interval)?
    • rename to just interval?
    • update tests

@casperdcl casperdcl marked this pull request as ready for review February 2, 2026 13:36
@casperdcl casperdcl changed the title update interval fix update_objective_interval Feb 2, 2026
@MargaretDuff
Copy link
Member

Some comments, without looking at the code:

  • What does update_objective_interval being 0 or infinity mean? I presume infinity means it is never calculated, but 0? We probably shouldn't allow 0 because mod 0 is not usually defined, at python raises an error ZeroDivisionError: integer modulo by zero
  • In general, I am up for update_objective_interval to be in the same place as the callbacks so in run. However, I think I would then want to store in which iterations the objective was calculated, so perhaps algorithm.objective could return both the list of objective values but also a corresponding list for in which iterations these were calculated. Then one could easily pass this to a plotting function and users could happily run for 5 iterations, check it doesn't crash and see that the objective value is decreasing, and then run for another 100 with a larger update interval, without having to keep track of it themselves.

@casperdcl
Copy link
Member Author

casperdcl commented Feb 2, 2026

  • update_objective_interval=0 was documented as being supported and was (inconsistently) used in the code to seemingly have the same intention as np.inf
  • moving update_objective_interval to run():
    • pro: we already moved (max_)iteration(s) to run()
    • con: update_objective_interval might affect the algorithm behaviour itself and thus should remain in __init__?
  • changing/adding a new @property objective: dict - might be worth opening a separate issue

@MargaretDuff
Copy link
Member

  • update_objective_interval=0 was documented as being supported and was (inconsistently) used in the code to seemingly have the same intention as np.inf

  • moving update_objective_interval to run():

    • pro: we already moved (max_)iteration(s) to run()
    • con: update_objective_interval might affect the algorithm behaviour itself and thus should remain in __init__?
  • changing/adding a new @property objective: dict - might be worth opening a separate issue

Thanks Casper, let's focus on the 0 and np.inf support in this PR and have a seperate issue and PR for moving update_objective_interval into run along with saving the interations where the objective value was calculated.

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