DiBB Overhaul #1
Conversation
… local mode Now also supported: - Callable class instances (implementing `__call__`) - Classes that implement `__call__ ` but are not yet instantiated (become a ray actor, yet to be tested)
It is directly related to the evaluation on the block worker, therefore it is a better fit.
giuse
left a comment
giuse
left a comment
There was a problem hiding this comment.
Thanks for bringing DiBB up to speed! I really like this. There's one file that should not be committed (and added to .gitignore), and one stylistic change I would rather roll back (switching all python strings to double quotes, I use single quotes to distinguish symbols, which are otherwise missing from python), otherwise all good and very happy with this! :)
| @@ -0,0 +1,33 @@ | |||
| # file generated by setuptools-scm | |||
| # don't change, don't track in version control | |||
|
|
||
| import numpy as np | ||
| # pip install cma -- https://pypi.org/project/cma | ||
| # Silence CMA warnings |
|
|
||
| def dump(self, fname): | ||
| with open(fname, 'wb') as fd: | ||
| with open(fname, "wb") as fd: |
There was a problem hiding this comment.
Mmm not really a fan of this... I utilize single quotes for "simple, constant strings" (because Python does not have a symbol type), and double quotes for "text or interpolated strings". Visually if there are single quotes I know it should be a value I can immediately recognize as a standard or symbol of some type, and just look for typos, while with double quotes I know I need to actually read the content and make sense of it. So, personally, I dislike having "wb" because I stop and decode the thing, while with 'wb' my eyes, just quickly check it's a write and binary because that's a open standard.
My point being: can we remove only this check from ruff? Would you agree to my standard? Otherwise I don't mind the other changes in style such as splitting one-liners in line 47.
| self.ndims = len(init_mu) | ||
| self.init_sigma = opts.pop('init_sigma', 0.5) | ||
| self.popsize = opts.pop('popsize', None) | ||
| self.init_sigma = opts.pop("init_sigma", 0.5) |
There was a problem hiding this comment.
Yeah these are also 'symbols' for me, not "text", so that's weird :)
| config = { | ||
| # initial candidate solution (flattened) | ||
| 'init_ind' : self.fit_fn.init_ind, | ||
| "init_ind": self.fit_fn.init_ind, |
There was a problem hiding this comment.
Also this for example looks super strange to me, keys should be symbols not text :) (and from an implementation perspective they are: python strings are immutable and hashed, so you can say python lacks a string type rather than a symbol type :) )
|
|
||
| self.assertEqual(rfit(1, rseed=self.rseed), | ||
| rfit(2, rseed=self.rseed)) | ||
| self.assertEqual(rfit(1, rseed=self.rseed), rfit(2, rseed=self.rseed)) |
There was a problem hiding this comment.
See, here I disagree with the changes because for me legibility trumps standards when the improvement is measurably significant. But can you tell ruff to have just this one exception? Especially calling it as a commit hook with non-interactive --fix sounds like a bit of a gamble to me.
| *.patch | ||
| *.diff | ||
| pyproject.toml | ||
|
|
2 participants
This PR includes the following changes: