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

Fix outdated use of camelCase names, et cetera #128

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

danielwe
Copy link
Contributor

Phew! I just wanted to try the Makie plot recipe, but it errored on calling getBranch instead of get_branch. I know you've been updating the naming conventions and that this is just an oversight, so I looked over the code and fixed all the leftover camelCase names I could find.

I found some other wrinkles along the way that are also fixed to the best of my ability in this PR

  • Some methods of continuation were written using the keyword normN, and some examples were using this calling convention. However, reading the tests and other continuation methods, I believe the intention is to use normC in continuation and bifurcationdiagram et cetera, while normN is the name of the kwarg in newton and predictor. I updated the code accordingly.
  • A couple of abstract types were not capitalized. Fixed that.
    • Though it seems like the AbstractModulatedWave* types aren't used anyway?
  • Some example code and a docstring were referring to an old tangentAlgo keyword that I believe has been refactored into PALC(tangent=...). I tried to update the docstring and example code accordingly.

Where possible I tried to update commented-out lines of code, but I skipped commented sections that span entire files or large parts of files.

@rveltz
Copy link
Member

rveltz commented Nov 14, 2023

Thanks a lot for doing this, that' an amazing work.
Fixing the camelCase names took a lot of toll on me because I had to make sure the test were not breaking silently.
I will review this as fast as I can.

Some methods of continuation were written using the keyword normN, and some examples were using this calling convention. However, reading the tests and other continuation methods, I believe the intention is to use normC in continuation and bifurcationdiagram et cetera, while normN is the name of the kwarg in newton and predictor. I updated the code accordingly.

Exactly, you are correct. Good catch.

One thing I did not push is a warning to tell the user when kwargs are undefined. You would have catched NormN easily.

Though it seems like the AbstractModulatedWave* types aren't used anyway?

I dont think it is. I planned to develop the waves a lot more but I might do it in a separate package.

@rveltz
Copy link
Member

rveltz commented Nov 14, 2023

Phew! I just wanted to try the Makie plot recipe

This is kinda working but it is a bit clunky. The plot_solution definition is different from the one for Plots. Additionnally, I stopped developping the current one because I would like to write one with observables and that's beyond my knowledge for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch for the tol_stability!!!

@rveltz rveltz merged commit d8bfa3d into bifurcationkit:master Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants