Closed neiljdo closed 1 year ago
Hi Neil, thanks for your submission, it looks quite good already. I've enabled the tests to run on this PR, so would you be able to have a look at where they're failing and see if you could fix them up?
Hi @ianyfan, thank you for the review. I just pushed a new commit to address the flake8
violations. I'll wait for the workflow run results to see if I missed some.
@ianyfan just saw the failing jobs related to the notebook tests and type checking - will push a fix to those soon.
@ianyfan just saw the failing jobs related to the notebook tests and type checking - will push a fix to those soon.
@neiljdo Hi, it would be great if you could that by tomorrow, 13/6, which is the last day of the hackathon.
@dimkart @neiljdo PRs can be reviewed also shortly after June 13th and still be awarded a bounty in uHACK if accepted.
@nathanshammah
PRs can be reviewed also shortly after June 13th and still be awarded a bounty in uHACK if accepted.
Thanks, noted.
Would also be great if you could run the clean_notebooks.py script, which removes some metadata from the examples and tutorials.
@nikhilkhatri, Thank you for the review. Here are my thoughts on your comments above.
ob_map
, and other ansatz-specific kwargs) required to initialize the ansatz during trainer checkpoint creation and re-initialize the ansatz during checkpoint load. I thought the ansatz deterministic and could be re-instantiated, so I went this route. I would like to do tests as part of the testing suite (not manually) - this implies adding new tests that use different ansatzes for the trainers.For (3) and (4) above, I realized that the model is coupled with the circuits while working on the issue. I saw this more while updating the demo notebooks and saw that the Model.from_diagram
call also needs to get the test circuits. Let's discuss these two later once I've resolved the first two.
@neiljdo you are correct in that the ansatze are deterministic. My point was to ensure that there are no Exceptions raised when pickling an ansatz class (for example, if they use a lambda function).
w.r.t the lambda pickling, your code may be alright since it pickles the class, not the instance. It would still be good to make sure.
May I drop a suggestion here? Based on what I understand from the code :
1) If Model is from checkpoint, so you just skip the _init_model_from_datasets method , but still have to recreate the circuits at training_step and validation_step (assuming number 1 is fixed).
2) If Model is not from checkpoint, you make the circuits at both the _init_model_from_datasets method and the training_step and validation_step.
Since the dataset method is not run every time, I think it could be better if you make the circuits once, pass the list to _init_model_from_datasets method, and also use the list in the training and validation step (based on the index of the diagram of course).
My suggestion is after fixing 1, to have the _init_model_from_datasets also have the reference from 1's fix. @nikhilkhatri dear, am I correct? Please correct me if I am wrong.
Hi @ACE07-Sev ,
Thanks for your comments. Indeed, the circuits should only be created once, and then used wherever the ansatz is currently applied. The circuits need to be created wherever the dataset and ansatz parameters are first available. If the dataset is taken as input first during fit
, then this is where the ansatz can be applied, and circuits from this can be reused throughout the setup, training and validation.
Hi @nikhilkhatri,
I've addressed points (1) and (2) from your original comment in my latest update. Specifically,
Trainer.fit()
execution. Circuits are also included in the checkpoint.dill
as a drop-in replacement for pickle
for checkpoint generation as it handles lambda functions well. I've added tests for all available ansatzes to make sure that it still generates the correct output after serialization-deserialization. With this, we can simplify the ansatz checkpoint creation i.e. no need to re-instantiate.Let me know if there are still issues, thank you very much!
EDIT: I ran the clean_notebooks.py
script but it did not update the notebooks.
Hi @neiljdo Thanks a lot for these changes!
I have a couple of comments:
fit
, at which point they can provide the necessary circuits.dill
hasnt been added to requirements. It would be great if you could address issues 1 and 2 for the hack.
@nikhilkhatri Noted, thanks. I'll remove the circuits from the checkpoint.
Neil dear :
lambeq/training/checkpoint.py:27: error: Cannot find implementation or library stub for module named "dill" [import]
The test with pytest passed, so I think you just need to fix this import error.
@neiljdo Hey Neil please leave a comment in the Issue #86, so you can be assigned to it (otherwise your handle cannot be found).
Moved to lambeq's private repository for further development. This feature will be included in one of the upcoming releases.
PR adds the ansatz class and necessary arguments for instantiation (e.g.
ob_map
and ansatz-specific kwargs) as hyperparameters to the trainer class. The functions related to creating and loading checkpoints have also been updated to accommodate the new ansatz class hyperparameter.Addresses https://github.com/CQCL/lambeq/issues/86.