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

Define apply function that does not modify arguments #30

Closed
wants to merge 1 commit into from

Conversation

apkille
Copy link
Contributor

@apkille apkille commented Sep 2, 2024

In preparation for the Gaussian quantum information package. apply would be useful to define here, as other packages in QuantumSavory use apply!.

@apkille
Copy link
Contributor Author

apkille commented Sep 2, 2024

@Krastanov pinging for review.

@Krastanov Krastanov self-requested a review September 4, 2024 19:16
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.29%. Comparing base (521a05b) to head (4cda081).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #30   +/-   ##
=======================================
  Coverage   17.29%   17.29%           
=======================================
  Files          12       12           
  Lines         399      399           
=======================================
  Hits           69       69           
  Misses        330      330           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@Krastanov Krastanov left a comment

Choose a reason for hiding this comment

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

I think this should just be *. That is the equivalent used in QuantumOptics and QuantumClifford.
Also, if it just happens that you have a structure that is not capable of in-place modifications, it is still fine to use a method with a ! in the name (for the sake of having a small API). In general, the safe way to use an in-place modifying function would be to write something like

my_structure = my_function!(my_structure)

Sure, this is redundant for the cases where my_structure is mutable, but it is pretty useful when down the line you need to introduce a structure that is not mutable without introducing a new API.

Regrettably, this is just a convention -- there is no way to enforce it in this language yet.

Thus, my preference would be to close this PR

Apologies for the slow response.

@apkille
Copy link
Contributor Author

apkille commented Sep 8, 2024

@Krastanov You make fair points. My only reasoning for introducing this into the API was for completeness. There's probably exceptions to this rule, but IMO if you have an in-place function you should usually have a corresponding out-of-place function. Since it's already implemented in QO and QuantumClifford, I agree with just using *.

@apkille apkille closed this Sep 8, 2024
@Krastanov
Copy link
Collaborator

yup, good point. As a precedent, * and mul! from LinearAlgebra follow the same separation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants