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

Huge refactor #309

Merged
merged 13 commits into from
May 11, 2021
Merged

Huge refactor #309

merged 13 commits into from
May 11, 2021

Conversation

zStupan
Copy link
Contributor

@zStupan zStupan commented May 7, 2021

Summary

  • Added python3.9 to integration tests
  • Renamed package to lowercase i. e. 'niapy'
  • Changed indentation to 4 spaces instead of tabs
  • Renamed variables and methods to snake case.
  • Fixed typos in docstrings.
  • Added missing docstrings and completed incomplete ones.
  • Temporarily removed (commented out) algorithms that weren't implemented or weren't working.
  • Renamed most of the algorithm parameters to names I think make sense.
  • Moved the initialization of algorithm parameters to the init method.
  • Removed type_parameters method from all algorithms and all the test cases for it.
  • Removed niapy.algorithms.statistics module as I don't think it's particularly useful.
  • Removed the utility classes in niapy.task.utility and niapy.algorithms.utility and replaced them with factory functions get_algorithm and get_benchmark in niapy.util.factory.
  • Made the task package into a module, since the package only contained one file (task.py) along with init.py.
  • Fixed issues Several TODOs in ca.py #306, limit_repair method alters the input array #294, CuckooSearch's runIteration is incompatible with other algorithms runIteration #281, """ #264, [JOSS] (Optional) Follow PEP-8 style guide in naming methods #123

Remaining problems:

  • I did not apply the code style changes and rules to tests.
  • I'm sure I missed some docstrings.
  • A lot of algorithms are missing reference papers,
  • All algorithms need to be checked if they're implemented correctly, I went through the reference papers for most of them and I think most are correct, I'm not sure about CRO, Krill Herd, and CrowdingDE.
  • There are some warnings when building the docs. Also things like the optimization tasks are missing in the docs. I don't know how to add them.
  • I'm sure a lot of performance could be gained by using numpy more efficiently, although performance isn't really the main goal of this project, so it's not a problem really.

I'm sorry it took so long 😅

@firefly-cpp
Copy link
Contributor

Awesome @zStupan!

Your commit looks brilliant. We are now just step away from v 2.0.

Please give me an answer on two additional questions (so that we can track changes easily).

  1. Are there any major changes regarding the Stopping-, Counting tasks?
  2. Did you check also termination conditions? Are they working correctly? Tests seem to be OK.

@firefly-cpp
Copy link
Contributor

Regarding the docstrings, I will enhance tests with interrogate package: https://github.com/econchick/interrogate

It may be very helpful for us.

What do you think?

@firefly-cpp
Copy link
Contributor

@zStupan - I forgot to say thank you for your time and effort. 👍

@zStupan
Copy link
Contributor Author

zStupan commented May 7, 2021

@firefly-cpp No problem.

Regarding Tasks, attribute names were changed, and the methods for getting lower, upper, range, iters and evals were removed as they can be accessed as attributes. They should work as expected.

Interrogate seems like a great addition.

@firefly-cpp
Copy link
Contributor

@GregaVrbancic any comments from your side? Otherwise, we can merge to master.

@rhododendrom
Copy link
Contributor

@zStupan Great job, huge enhancement. Many thanks. Respect.

@sisco0
Copy link
Contributor

sisco0 commented May 7, 2021

Thank you for the effort in the project, @zStupan.

I could see the summary changes you mentioned in the commits referred.
Merging is right with me.

Copy link
Contributor

@sisco0 sisco0 left a comment

Choose a reason for hiding this comment

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

These changes would benefit the current project structure by removing unneeded nested repeated package names.

@GregaVrbancic
Copy link
Contributor

@GregaVrbancic any comments from your side? Otherwise, we can merge to master.

It looks perfect to me. Superb job @zStupan!

@GregaVrbancic
Copy link
Contributor

@all-contributors please add @zStupan as for doc, example and test contributor

@allcontributors
Copy link
Contributor

@GregaVrbancic

I've put up a pull request to add @zStupan! 🎉

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.

5 participants