Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Disable unnecessary transformations and cleaning up API#331

Merged
bettinaheim merged 157 commits intofeatures/transformationSetupfrom
beheim/transformationNodes
Feb 26, 2020
Merged

Disable unnecessary transformations and cleaning up API#331
bettinaheim merged 157 commits intofeatures/transformationSetupfrom
beheim/transformationNodes

Conversation

@bettinaheim
Copy link
Contributor

@bettinaheim bettinaheim commented Feb 24, 2020

Since we do a pretty heavy breaking change for our transformation setup, I went ahead and also made a pass to clean up the API.
This PR also disables subtransformations that are not needed.

Bettina Heim added 30 commits February 10, 2020 17:04
…check IdentifierReferences transformation!
Copy link
Contributor

@ScottCarda-MS ScottCarda-MS left a comment

Choose a reason for hiding this comment

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

Please revert the renaming for the Classically Controlled, Monomorphization, Monomorphization Validation, and Intrinsic Resolution transformations, as well as the changes to the Utility class, so that the changes can be placed in a separate PR.

Copy link
Contributor

@cesarzc cesarzc left a comment

Choose a reason for hiding this comment

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

Renaming really made the code more readable, this is great!

@bettinaheim
Copy link
Contributor Author

bettinaheim commented Feb 25, 2020

Please revert the renaming for the Classically Controlled, Monomorphization, Monomorphization Validation, and Intrinsic Resolution transformations, as well as the changes to the Utility class, so that the changes can be placed in a separate PR.

@ScottCarda-MS What is the reason to want it a separate PR? There is no benefit to that in terms of resolving merge conflicts, and logically the API clean up belongs here.

Copy link
Contributor

@bamarsha bamarsha left a comment

Choose a reason for hiding this comment

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

Thanks for capitalizing all the method names!



namespace Microsoft.Quantum.QsCompiler.Transformations.IntrinsicResolutionTransformation
namespace Microsoft.Quantum.QsCompiler.Transformations.IntrinsicResolution
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the file be renamed to IntrinsicResolution.cs as well? The naming convention in this folder seems split between files that end in Transformation and those that don't. Since the namespaces are being shortened it could make sense to shorten the filenames too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My preference would be to rename the files, too, dropping the *Transformation ending. It seems silly to have all files within a folder named Transformations ending in Transformations as well. The reason they were named this way I believe is that there is a RewriteStep folder in the Compiler project that contains files with the same name which implement the IRewriteStep interface invoking these transformations. Personally, I think having both files have the same name is fine.

Copy link
Contributor

@bamarsha bamarsha Feb 26, 2020

Choose a reason for hiding this comment

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

Oh, I see. Yes, I think it's fine to have the same filenames since it should be clear from context which one is the transformation. If we don't do that then for consistency I'd argue that Conjugations.cs, FunctorGeneration.cs, etc. should be renamed to ConjugationsTransformation.cs, FunctorGenerationTransformation.cs, etc. I prefer the shorter names. :)

Then we also have files like BasicTransformations.cs and CodeTransformations.cs, but having files named Basic.cs and Code.cs would probably be confusing.

bettinaheim and others added 2 commits February 26, 2020 08:02
Co-Authored-By: Sarah Marshall <33814365+marshallsa@users.noreply.github.com>
…on.cs

Co-Authored-By: Sarah Marshall <33814365+marshallsa@users.noreply.github.com>
@bettinaheim bettinaheim merged commit 200b357 into features/transformationSetup Feb 26, 2020
@bettinaheim bettinaheim deleted the beheim/transformationNodes branch February 26, 2020 18:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants