CUQI-DTU / CUQIpy

https://cuqi-dtu.github.io/CUQIpy/
Apache License 2.0
44 stars 9 forks source link

Allow samplers to be initialized without target #408

Closed nabriis closed 5 months ago

nabriis commented 6 months ago

Closes #398 (Target=None) Closes #397 (Proposal based refactor) Closes #414 (Reset method) Closes #401 (Initial point in sample)

Change overview 1) Added _initialize abstractmethod that all subclasses must implement. This method is called before sampling and allows "lazy" initialization allowing target not be set during init. 2) Updated all samplers to use _initialize and allow Target=None. 3) Refactoring proposal implementation to be aligned with target (including validate_proposal) 4) Fixed the reset method to properly reset using the new _initialize method 5) Removed initial point from samples 6) Added gitignore on CUQI_samples

Area of interest for reviewer

nabriis commented 5 months ago

Many thanks for this extensive PR. I have gone through it and as far as I can see it achieves the intentions and closes the four listed issues, so that is really great. There is quite a bit of technical details and I have looked through and do not have any specific comments, so my review is here is not a technical review. I see @amal-ghamdi have provided extensive comments that all appear to have been addressed successfully.

My only comment is in relation to the naming, as requested. I agree that "initialize" could cause confusion with initial point, especially since the initial point is not part of initialization. The suggestion of "set" is good and would go well with "reset". Also it makes sense to talk about the sampler being in a "set" or "unset" mode, and the "set" method would take it into the "set" mode. I think one thing we have learned is that it is helpful for users (and ourselves) to be quite explicit, and perhaps we can be more explicit than "set", and also avoid a slight confusion since "set" is both the method name and the mode name.

One possibility would be "configure". This makes it more explicit, and like "set" decouples it from the start/initial phase of the sampler. So the method would be "configure" and the mode would be "configured", with a slightly better distinction. "Reset" would then be "reconfigure".

Some things to consider:

  1. Do we need the "re" at all? Could simply "configure" be enough? If calling "configure" on an unconfigured sampler would configure it, and on an already configured sampler would also configure it by replacing the existing configuration by the new one, with the resulting sampler being the same whether or not it was already configured.
  2. Whatever the word "configure", "set" or "initialize" I think it is not completely clear that that amounts to setting the state and history. I understand that earlier there was "initialize_state" and "initialize_history", which appear more explicit and thus perhaps clearer? Otherwise the documentation could clarify the definitions and connections between being configured, state and history and distinction vs parameters and other concepts.

Hi @jakobsj. As we discussed I have updated the name for reset to reinitialize and kept the other names the same. I also updated the documentation in the classes to make thing more clear (hopefully). Finally I added a nice test for the reinitialization that ensures subclasses implement it consistantly.

nabriis commented 5 months ago

I like these changes and I think we are getting close. I have added a few comments, if you have time it would be great if you can try to address them before we meet today, and then we can progress it further then.

I adressed your comments. Thanks. It helped improve the clarity @jakobsj