Fix of heralds display (issue 207)

[burlemarxiste]

This is a separate branch for this fix.

https://github.com/Quandela/Perceval/issues/207

[ericbrts]

Hello @burlemarxiste , I've created an integration branch for UnitaryHACK 2024 contributions. I've just merged your Barrier PR. It seems it created conflicts. Could you please solve them before I review this PR?

I'm assigning the bounty to you right now.

[burlemarxiste]

Sure! I will look into this tomorrow!

[ericbrts]

Thanks, testing and reviewing it right now!

[burlemarxiste]

I'm a bit lost with the branches at this moment. How should I proceed to continue making changes related to this issue, but with the barrier implementation included to it?

Can I merge the barriers code accepted in Quandela:release/unitary-hack-2024 and update this pull request?

[ericbrts]

You fix_heralds branch seems to be the correct source branch to work on. I'm messing with destination branches, because I want to enable the automated test workflow (which I failed to do). In the end, your synchronized with main + your Barrier branch, so working on fix_heralds is the way to go.

I'll merge it in an intermediate branch (right now, named "release/unitary-hack-2024") so that I can control to which scope we add these unitary hack contributions.

[burlemarxiste]

Is it okay if I merge the latest version of my barrier branch into this? It would be easier to regenerate the figure for the test that fails. I might also add a few more tests (circuits with recursive True or False).

[ericbrts]

The Barrier code I merged on Friday is already in your fix_heralds branch (or do you have newer code for Barriers?). I made you split your work into 3 different PR for easier reviews. Now we're reconstructing your work brick by brick. When you merged fix_heralds from the unitary-hack-2024 branch, you retrieved your work about Barriers:

[burlemarxiste]

Thank you for confirming! I have made the changes you suggested to check for null object width, updated the image, and fixed the problem with the missing background color (it was a line that disappeared when I merged with the barrier branch).

[burlemarxiste]

I have found an issue with my implementation: collect_herald_info should collect information with the same level of flattening as the diagram to be drawn.

Can you confirm that this is how the diagram should look when recursive=False?

[ericbrts]

Yes you can add a recursion_level parameter with a default value meaning max recursion level (like 0 ? -1 ?)

The picture for the non-recursive display is totally fine.

[burlemarxiste]

Thanks for confirming that the image is correct.

I just understood something that answered my initial confusion about the recursive argument: there are only 2 possible recursion levels. The reason is that when a Processor is added to a Processor, they are composed (no nesting between them); and iterating on a circuit always goes to the deepest level.

So there are only two possible levels, and there is no needed for an extra argument. I just need to collect the herald info at the component level when recursive=False, and circuit element level when recursive=True. (Processors are composed, but circuits can remain nested with the argument merge=False).

This latest commit should fix it.

[ericbrts]

Thanks @burlemarxiste , I'm reviewing / testing again now. Do you wish to add other things to your PR, or can I merge once I approve it?

[burlemarxiste]

I'll leave it as is for the Heralds display bug fix, but when investigating on nesting levels, I have found an unrelated display issue (present before I did any change, not affected by my changes). Should I file an issue for it?

[ericbrts]

Yes please, file as many issues as you wish!