Allow callables directly in the simulation model#254
Allow callables directly in the simulation model#254gilrrei wants to merge 1 commit intoqueens-py:mainfrom
Conversation
| self.scheduler.copy_files_to_experiment_dir(self.driver.files_to_copy) | ||
| self.function: SchedulerCallableSignature | ||
| if isinstance(driver, Driver): | ||
| self.function = driver.run_from_parameters |
There was a problem hiding this comment.
With this, I reverted the __call__ method, since we need to check if it is a driver either way. This way, we can then see the function that is being called directly.
| Result and potentially the gradient | ||
| """ | ||
| return self.run(sample, job_id, num_procs, experiment_dir, experiment_name) | ||
| def run_from_parameters( |
There was a problem hiding this comment.
Maybe we should add a default to the parameters like parameters=None in the init of the driver to indicate that parameters are not necessarily needed anymore? If we decide to do that, this function here needs to check that the parameters are not None before calling sample_as_dict.
gilrrei
force-pushed
the
allow_function_in_simulation_model
branch
2 times, most recently
from
04ff736 to
055d99c
Compare
gilrrei
force-pushed
the
allow_function_in_simulation_model
branch
from
055d99c to
cc96d3b
Compare
|
Looks good! I think this would also close #233, since the only use case for a |
| self.function: SchedulerCallableSignature | ||
| if isinstance(driver, Driver): | ||
| self.function = driver.run_from_parameters | ||
| self.scheduler.copy_files_to_experiment_dir(driver.files_to_copy) | ||
| else: | ||
| self.function = driver |
There was a problem hiding this comment.
As I mentioned to you in person my personal preference would be this version and keeping the __call__ method.
| self.function: SchedulerCallableSignature | |
| if isinstance(driver, Driver): | |
| self.function = driver.run_from_parameters | |
| self.scheduler.copy_files_to_experiment_dir(driver.files_to_copy) | |
| else: | |
| self.function = driver | |
| self.function = driver | |
| if isinstance(driver, Driver): | |
| self.scheduler.copy_files_to_experiment_dir(driver.files_to_copy) |
leahaeusel
left a comment
leahaeusel
left a comment
There was a problem hiding this comment.
The changes look good to me, thank you for taking care of this! 🚀 Just two small comments from my side.
| scheduler (Scheduler): Scheduler for the simulations | ||
| driver (Driver): Driver for the simulations | ||
| """ | ||
| """Simulation model class.""" |
There was a problem hiding this comment.
| """Simulation model class.""" | |
| """Simulation model class. | |
| Attributes: | |
| function: Callable for the simulations | |
| """ |
| Result and potentially the gradient | ||
| """ | ||
| return self.run(sample, job_id, num_procs, experiment_dir, experiment_name) | ||
| def run_from_parameters( |
There was a problem hiding this comment.
Maybe we should add a default to the parameters like parameters=None in the init of the driver to indicate that parameters are not necessarily needed anymore? If we decide to do that, this function here needs to check that the parameters are not None before calling sample_as_dict.
4 participants
Description and Context:
What and Why?
This PR enables the direct use of functions in the simulation model, provided they have a specific signature. It follows from discussion #244 and the related meeting.
(As always, better naming welcome)
Related Issues and Pull Requests
Interested Parties