Skip to content

Added new spiral part where length can be set#14

Open
fbeutel wants to merge 11 commits into
HelgeGehring:masterfrom
fbeutel:new_spiral
Open

Added new spiral part where length can be set#14
fbeutel wants to merge 11 commits into
HelgeGehring:masterfrom
fbeutel:new_spiral

Conversation

@fbeutel

@fbeutel fbeutel commented Nov 13, 2019

Copy link
Copy Markdown
Contributor

This replaces the old spiral with a new version of an archimedean spiral which allows to set the length. The number of turns required to achieve the length is determined numerically (afaik an analytical solution doesn't exist).

Different types are supported. Especially for use inside an MZI the inline and inline_rel types are of interest.
spiral

ToDo's before merge:

  • Update changelog
  • Add to tests
  • Update documentation
  • Allow choosing the spiral direction (752d3b2)
  • Choose an appropriate guess value for the numerical estimation

@HelgeGehring

HelgeGehring commented Nov 13, 2019

Copy link
Copy Markdown
Owner

Nice Spiral!

  • Could you remove the (object) in the line where you define the class? It was only necessary in python 2 (see link)

About backward compatibility: I'd guess it would be good to first deprecate the old spiral and have both for one version. Afterwards, we remove the old one and then also replace the name?

@fbeutel

fbeutel commented Nov 13, 2019

Copy link
Copy Markdown
Contributor Author

Yeah, about backwards compatibility I'm also not sure. One could try to make it backwards compatible to the old spiral, but I think that would be a rather ugly hack, so maybe it's better to go with two spirals for now and replace the old one later on...

@HelgeGehring

Copy link
Copy Markdown
Owner

No, I think finally the spiral should just be replaced, so please no ugly hacks ;)
But for at least one version we should deprecate the old one and offer both. For the following release we then can remove the old spiral I'd say.
Besides that: I'd guess it would be a good idea to let all functions you define in the beginning start with an underline in order to keep the interface simple.

@HelgeGehring HelgeGehring added the enhancement New feature or request label Nov 13, 2019
@fengsuchun

Copy link
Copy Markdown

Could you add squared spiral with euler bend?

@fbeutel fbeutel marked this pull request as ready for review August 8, 2021 19:18
@fbeutel

fbeutel commented Aug 8, 2021

Copy link
Copy Markdown
Contributor Author

@HelgeGehring Can you add the changelog entry as you see fit? Other than that I think it's ready for review

@fbeutel fbeutel changed the title WIP: Added new spiral part where length can be set Added new spiral part where length can be set May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants