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

Switching out parameters in torus function #114

Merged
merged 1 commit into from
Jun 2, 2024
Merged

Switching out parameters in torus function #114

merged 1 commit into from
Jun 2, 2024

Conversation

odedstein
Copy link
Collaborator

This is in response to bug report #113 .
The parameters nr and nR are switched in the torus.py function.

@odedstein
Copy link
Collaborator Author

@sgsellan do you think this is the correct change to make? Or should I have changed the names of the parameters in the function declaration, but kept the old order, so as to not break old code?

@sgsellan
Copy link
Owner

Mmmm yeah, now that you mention it, it may be a better idea to keep backwards compatibility with old code (otherwise if I remember our rule correctly, that'd mean that we can only merge this PR for the next major release, 0.3.0)

@odedstein
Copy link
Collaborator Author

I was just going to make the change, but then the new order of parameters nr,nR,R,r would be weird unless we also change the order at the end of R,r to r,R (another breaking change).
Let's maybe just wait with merging this until we do 0.3.0 - it's a minor fix that should not be too hard to merge.

@sgsellan
Copy link
Owner

Sounds good to me then! Let's wait to merge this

@sgsellan sgsellan added ready to merge This PR is finished and will be merged for the next release breaking change This PR is finished but includes a breaking change so it will be merged for the next 0.x.0 release labels Feb 15, 2024
@odedstein odedstein merged commit 3dc8d8b into main Jun 2, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This PR is finished but includes a breaking change so it will be merged for the next 0.x.0 release ready to merge This PR is finished and will be merged for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants