WIP: initial cleanup - Githubissues

scikit-hep / pyBumpHunter

Python implementation of the BumpHunter algorithm used by HEP community.

BSD 3-Clause "New" or "Revised" License

17 stars 9 forks source link

WIP: initial cleanup #4

Closed jonas-eschle closed 3 years ago

jonas-eschle commented 3 years ago

Hi @lovaslin, as promised here is a sketch of the cleanup and renaming. Needless to say that I fully agree also with @henryiii comment in #3, so feel free to use the cookie cutter.

The code is not cleaned up throughout and maybe I have also done a change too much, but it shows the direction. Try to follow the examples (e.g. Reset -> reset) etc.

The superclass (with the bad name BumpHunterInterface) is an example: maybe there can be more extracted than just having abstract names.

See also comments in my review that follows shortly

And let us know if something is unclean!

jonas-eschle commented 3 years ago

I've added a few comments and suggestions. If you disagree, please just write! Maybe others have other ideas about it, however I think the main direction is in agreement.

jonas-eschle commented 3 years ago

Also, maybe check the input arguments if they are valid.

You can work on this branch if you want, I am gonna leave it from now on. As mentioned, just ping us in case you have any question/remark/disagree with a comment

eduardo-rodrigues commented 3 years ago

Hi, excellent to see this MR! There is a lot already so I won't be reviewing on top of this. Just one comment: maybe the time already to move the testing directory from testing to tests, which is far more standard?

jonas-eschle commented 3 years ago

Ah indeed, I was not careful enough. What you can do is to expand the decorator: check if the deprecated name is in the kwargs (if it is positional, it's fine).

P.S: Very good progress!

jonas-eschle commented 3 years ago

Here I started to work on the docstring. I tried to keep a consistent way of formatting them, though I don't think it complies with PEP257 yet. It is just the first step.

That looks already good. I think there are a few things that come into what decides the exact formatting.

docs: that's an open point that I did not mention yet, which can be done using sphinx (and sphinx autodoc. There are a few styles for docs, numpy and google are the mostly used ones and sphinx (napoleon) is an extension that converts them to rst. What you have now is closely to the numpy doc style.
type-hints: so we want to communicate the types to the user. This can be done in the docs (see styles) or (better) as type hints as these are also machine readable an allow for automatic checks.

lovaslin commented 3 years ago

I would suggest to merge this branch with master now so I can release the change we did to the API. Then we can keep this branch up to continue working on it. Is there anything else that should be added to the repository (code of conduct, badges on the README, ...) ?

jonas-eschle commented 3 years ago

Since we have many discussions open here, I would prefer to leave the PR open and maybe make a new branch from this and a PR (e.g. 'updated API' or similar). What do you think?

lovaslin commented 3 years ago

I agree, I will do it now. Thank you for help and advice !