convexengineering / gpfit

Fit posynomials to data
http://gpfit.readthedocs.io/en/latest/
MIT License
10 stars 7 forks source link

v0.1 refactor #95

Closed pgkirsch closed 3 years ago

pgkirsch commented 3 years ago

I propose we create a 0.0 release of GPfit and then perform a pretty wholesale refactor of the code. Here is the list of changes I would like to make:

There is a little overlap with #73 but would be good to include those items too!

@bqpd @whoburg any objections or things you want to add?

bqpd commented 3 years ago

Those look good to me! I might just add "write the Getting Started docs page"?

pgkirsch commented 3 years ago

Good call. The docs need a lot of love but that seems like an appropriate piece to include in this effort.

bqpd commented 3 years ago

The cleaner the API the shorter that page will be, too!

whoburg commented 3 years ago

@pgkirsch this sounds fantastic! What are your thoughts on using a code formatter like black to accomplish some of the earlier bullets?

pgkirsch commented 3 years ago

Thanks @whoburg! black seems cool, I'll give it a go.

pgkirsch commented 3 years ago

@whoburg I ran black on the code. It certainly brings nice uniformity and saves time spent mindlessly formatting code.

There are a few things I don't love though. For instance, I'm not a fan of its policy on whitespace around *, /, ** operators. I believe PEP allows a = x*y + z**2 and I've always thought that looks nicer and more concise than a = x * y + z ** 2.

It also does makes some weird changes like:

-Vdd = random_sample(1000,) + 1
# ...
+Vdd = (
+    random_sample(
+        1000,
+    )
+    + 1
+)

Sure, that original line probably isn't great style and the black change can be fixed by assigning x = random_sample and doing Vdd = x + 1 but it still feels odd to enforce a change that seems to make the code less readable.

Lastly, rather than making the code more concise, it increased the total lines of code in the main directory very slightly from 906 to 941 (despite allowing lines up 88 chars long).

Anyway don't mean to complain too much, it's clearly a powerful tool and I'm on board with continuing to use it. I just wanted to flag a few minor things that gave me pause.

(cc @bqpd)

bqpd commented 3 years ago

Yeah the best use I've seen of black is to pretty-print long data entries for adding quick tests: just copy-paste in one line, then run black to line-break etc.; its style differs from our matlab-flavoured conventions quite a bit for arithmetic!

bqpd commented 3 years ago

(also it would've been super useful back when we were first linting everything!)

whoburg commented 3 years ago

Good discussion @pgkirsch and @bqpd, I'm all for whatever is most pleasing and efficient to use, whether that includes a code formatter or not. Completely up to you.

pgkirsch commented 3 months ago

Just coming back here to say that I've fully come around on black and would support using it not only in gpfit but also in gpkit, maybe once the great pylint refactor of 2024 is merged 👀