Incorrect AssertionError message in NPE.train()

[abelaba]

🐛 Bug Description

When using NPE and calling the train() method a second time, while setting both force_first_round_loss and resume_training arguments to False. The assertion set on this line is raised https://github.com/sbi-dev/sbi/blob/8762b0e3fdea8804a4327e286342891697cf7f46/sbi/inference/trainers/npe/npe_base.py#L619C10-L631C14.

The assertion message states that append_simulations(theta, x) was called after training the network, even though the append_simulations method wasn't used again after training the network.

🔄 Steps to Reproduce

1. Python version - 3.10.16 and SBI version - 0.24.0
2. Minimal code example that triggers the bug

  def simulator(theta):
      return 1.0 + theta + torch.randn(theta.shape, device=theta.device) * 0.1

  num_dim = 3
  prior = BoxUniform(low=-2 * torch.ones(num_dim), high=2 * torch.ones(num_dim))
  theta = prior.sample((300,))
  x = simulator(theta)

  inference = NPE(prior=prior)
  inference.append_simulations(theta, x)

  inference.train()
  inference.train(force_first_round_loss=False, resume_training=False)

[satwiksps]

Hi! @abelaba @janfb
I would like to work on this issue.

The problem here is not with the assertion condition itself, but with the misleading error message that suggests append_simulations() was called again after training, even though the user is simply calling train() a second time without resume_training=True.

I plan to fix this by:

• Updating the assertion message to correctly reflect the situation:
  showing that the network has already been trained and that the user should either
  (a) set resume_training=True to continue training the same model, or
  (b) call append_simulations() again to start a new round.
• Optionally making the guard condition clearer by explicitly checking for resume_training=False when _round > 0.
• Adding a regression test to ensure the updated error message is raised appropriately.

I'll open a PR soon once the fix and tests are ready.

[janfb]

The plan is good @satwiksps 👍

Regarding your point two: I am not 100% sure, but the resume_training option really only applies to the scenario of single-round NPE I think (please confirm @michaeldeistler ?).
So, when train() is called there are multiple user stories (and deviations should result in an appropriate error, e.g., RuntimeError instead or AssertionError):

• when no new data was appended and train is called again one should set resume_training=True; (applies only to round 0)
• if new data was appended and train is called, then either a proposal has to be passed with appending, or the user must explicitly set force_first_round_loss=True (applies to all rounds)

[michaeldeistler]

Generally, resume_training can be used for any method (including NLE and multi-round SNPE). The idea back then was that we wanted to support training times of more than 24h, but many compute clusters have a max time of 24h. resume_training=True should really just allow to continue training exactly where one left off (including the optimizer state etc).

when no new data was appended and train is called again one should set resume_training=True; (applies only to round 0)

when no new data was appended and train is called again one should set resume_training=True; (applies only to round 0)
if new data was appended and train is called, then either a proposal has to be passed with appending, or the user must explicitly set force_first_round_loss=True (applies to all rounds)

I agree with both of these things. Not sure if there is anything to change in the code. Maybe a more specific assertion message does the job.

More generally: the resume_training argument is one of these things that I would be fine with deleting and just pointing users to the training interface.

[satwiksps]

Thanks @janfb and @michaeldeistler for the clarification

Based on your suggestions, I'll slightly update the plan:

• The core issue is still that the raised assertion message is misleading.

• I will update the message so it correctly distinguishes between the two valid user stories:

  (1) When no new data was appended and train() is called again (round 0), the correct action is to set resume_training=True.
  (2) When new data is appended in later rounds, the user must either provide a proposal or explicitly set force_first_round_loss=True.

• No changes to the underlying logic, just clearer, more accurate error messages as suggested.

I'll proceed with the implementation and open a PR soon.