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

Added new spiral part where length can be set #14

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

fbeutel
Copy link
Contributor

@fbeutel fbeutel commented Nov 13, 2019

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
Copy link
Owner

HelgeGehring commented Nov 13, 2019

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
Copy link
Contributor Author

fbeutel commented Nov 13, 2019

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
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

Could you add squared spiral with euler bend?

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

fbeutel commented Aug 8, 2021

@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.

None yet

3 participants