Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(frontends): enable simultaneous execution and simulation in mod… #1016

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

aPere3
Copy link
Collaborator

@aPere3 aPere3 commented Aug 27, 2024

…ules

@cla-bot cla-bot bot added the cla-signed label Aug 27, 2024
@aPere3 aPere3 force-pushed the alex/module_simulation_execution branch 2 times, most recently from 30b4291 to fbcc83b Compare August 27, 2024 14:41
@aPere3 aPere3 force-pushed the alex/module_simulation_execution branch from fbcc83b to 4c06d87 Compare August 27, 2024 14:52
Copy link
Member

@BourgerieQuentin BourgerieQuentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the purpose of the Box stuff? Else look good to me, maybe we should refactor the circuit.py using the Lazy stuff.

@aPere3
Copy link
Collaborator Author

aPere3 commented Aug 29, 2024

What the purpose of the Box stuff? Else look good to me, maybe we should refactor the circuit.py using the Lazy stuff.

This is basically a reference. It allows botht the FheModule and all the FheFunctions to point to the same object.

Edit: Turns out this is not needed as objects are passed by reference.

@aPere3 aPere3 force-pushed the alex/module_simulation_execution branch from 4c06d87 to 323379f Compare August 29, 2024 08:46
Copy link
Contributor

@umut-sahin umut-sahin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation is okay, but it definitely needs tests.

  • Start with both disabled, enable both later
  • Start with just simulation, enable execution later
  • Start with just execution, enable simulation later
  • Start with both simulation and execution

@aPere3 aPere3 force-pushed the alex/module_simulation_execution branch from 323379f to 51f5480 Compare August 29, 2024 14:57
@aPere3
Copy link
Collaborator Author

aPere3 commented Aug 29, 2024

The implementation is okay, but it definitely needs tests.

  • Start with both disabled, enable both later
  • Start with just simulation, enable execution later
  • Start with just execution, enable simulation later
  • Start with both simulation and execution

Done.

@aPere3 aPere3 force-pushed the alex/module_simulation_execution branch 3 times, most recently from bf44209 to 7389ed2 Compare August 30, 2024 12:46
self._initialized = True

@property
def val(self) -> T:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion, but using value would be better IMO.

Copy link
Contributor

@umut-sahin umut-sahin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay to merge.

@aPere3 aPere3 force-pushed the alex/module_simulation_execution branch 5 times, most recently from 62eafb1 to af08e90 Compare September 2, 2024 12:44
@aPere3 aPere3 force-pushed the alex/module_simulation_execution branch from af08e90 to 52f98fe Compare September 2, 2024 13:58
@BourgerieQuentin BourgerieQuentin merged commit 165746d into main Sep 2, 2024
27 of 31 checks passed
@BourgerieQuentin BourgerieQuentin deleted the alex/module_simulation_execution branch September 2, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants