T anikda/experimental

[AniketDalvi]

The code changes made in this PR primarily revolve around implementing the logic to reorder the wavefunction. It also includes some supporting functions to implement this change. This reordering routine has been added as an effort to optimize the full-state native simulator.

[bettinaheim]

@AniketDalvi
We should keep this on the feature branch until all the bits and pieces have been implemented. I retargeted the PR; could you please resolve the merge conflicts? Thanks!

[IrinaYatsenko]

For performance improvements it's super useful to have tangible numbers that highlight the problem and show the fix is working (and the situations when the fix might not be applicable). Do we have benchmarks for the scenarios you are addressing? Can they be checked-in alongside with your change?

The PR doesn't link to any issues -- is there an issue tracking this perf improvements? Please add benchmark results to this PR's description and to the issue (if it exists).

[swernli]

I have a few suggestions for some low hanging optimizations and some minor formatting fixups, but otherwise the overall approach looks good!

[IrinaYatsenko]

Declaring setters as const doesn't make sense: the purpose of the setter is to change the object, isn't it?

Another problem with the signature is that it still does an extra unnecessary copy of Fusion object, first into the parameter when the function is invoked and then once again from the parameter into fusedgates. Change it to void set_fusedgates(const Fusion& newFusedGates) { fusedgates = newFusedGates; }

[AniketDalvi]

This needs to be set as const, as I am calling it in flush() which is const. Yes, the setter is supposed to change the object - but in this case by change here we are changing the contents of the container, not the container or its location.

[IrinaYatsenko]

As a part of public class interface a const setter doesn't make much sense, whatever the implementation details of other methods might be. If clients outside of the class need to use the setter (for example, tests), make it non-const for them and inside flush() do the assignment directly, without going through the setter.

[AniketDalvi]

Apologies, I did not really understand what you were trying to convey here completely. Unfortunately because the data structure fusedgates is private, I cannot make the assignments outside this class, thereby not allowing me to make assignments in flush() directly. Given this, the use of a setter is necessary to alter the datastructure. And as this setter is being called in a function which is declared const, the language does not allow me to have the setter to be non-const.

[IrinaYatsenko]

"funny" public interfaces like this usually indicate that there is an issue with design of data structures: basically, you are trying to put a square peg into a round hole. You have multiple choices to address it:

1. keep entrenching the previous design decision(s) and add public interfaces that don't conform to good practices (ugly)
2. keep the previous design decision intact and workaround by adding const_cast(s) (ugly but obvious to spot in code and keeps badness from spreading)
3. remove const from member functions that violate constness (easy, as any chickening out is, but honest :))
4. rethink the logical\representational constness model of your data structures and get it right (hard and, given where C++ is re constness, the results might not be that different from option 3)

Personally, for a quick solution I'd go with either 2 or 3.
Your call.

[AniketDalvi]

Thought about this a little. Given a couple of factors - what this branch is trying to achieve and considering that I have just a couple of weeks left, I think it will be good idea for me to create an issue outlining this very relevant issue you bring up which can be tackled later. The only reason I say that is because I feel this deviates a little from the original purpose of this branch, and that making this const change is going to take some time as it very deeply entrenched in the simulator codebase, and I am hoping to get a couple of more branches checked-in before the end of my internship, and want to leave enough time for that. Thank you!

[IrinaYatsenko]

I still believe that it would be better to remove const from flush - it just makes no sense that all members of a class are mutable...

[IrinaYatsenko]

Without the understanding of the impact of the reordering in the case of multiple fused groups I don't feel comfortable pushing this code into production. If you must merge it, please consider adding a configuration switch that allows running it locally (e.g. with benchmarks) but doesn't enable the code for the end users yet. There are also still a few specific comments that haven't been addressed.

However, assuming you've discussed this with others, I don't want to block the PR so I'm changing my status to "Approve changes" (as there seems to be no way to remove my change request without approving). This doesn't indicate my sign off, though. Please seek a sign off from your lead and/or from the people you worked with on the design of this change.