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

v0.1 refactor #95

Closed
17 tasks done
pgkirsch opened this issue May 3, 2021 · 10 comments · Fixed by #99
Closed
17 tasks done

v0.1 refactor #95

pgkirsch opened this issue May 3, 2021 · 10 comments · Fixed by #99
Assignees
Labels
Milestone

Comments

@pgkirsch
Copy link
Contributor

pgkirsch commented May 3, 2021

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:

  • General code tightening (more concise code, less unnecessary whitespace)
  • Py3 only (e.g. delete __future__ imports)
  • Delete legacy Matlab code
  • Better variable naming
    • Appropriate use of case (rename upper case and mixed case vars unless global)
  • Better function naming
    • Rename get_params
    • Rename ba_init
  • Better class naming
    • Rename FitCS
  • Code restructuring
    • MA, SMA, ISMA should all be in one module
    • Reduce confusing overlap of naming of fit.py and fit_constraintset.py
    • Separate FitCS and XfoilFit (move to xfoil.py)
    • Group lse_scaled and lse_implicit into one module
    • Fit should be a class not a function
    • Eliminate plot_fit.py and print_fit.py (they should be class methods)
  • Remove examples/tests redundancy
    • Better naming of the examples too
  • Use scipy implementation of Levenberg Marquardt #98
  • Write the Getting Started docs page

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?

@pgkirsch pgkirsch self-assigned this May 3, 2021
@bqpd
Copy link
Contributor

bqpd commented May 3, 2021

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

@pgkirsch
Copy link
Contributor Author

pgkirsch commented May 4, 2021

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

@bqpd
Copy link
Contributor

bqpd commented May 4, 2021 via email

@whoburg
Copy link
Collaborator

whoburg commented May 22, 2021

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

@pgkirsch
Copy link
Contributor Author

pgkirsch commented Jul 3, 2021

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

pgkirsch added a commit that referenced this issue Jul 6, 2021
pgkirsch added a commit that referenced this issue Jul 6, 2021
pgkirsch added a commit that referenced this issue Jul 6, 2021
Specify python 3.4+ required (#95)
pgkirsch added a commit that referenced this issue Jul 7, 2021
* Combine lse_implict and lse_scaled into a single module
* Consolidate test module too
pgkirsch added a commit that referenced this issue Jul 7, 2021
* FitCS --> FitConstraintSet
* fit_constraintset.py --> constraint_set.py
pgkirsch added a commit that referenced this issue Jul 9, 2021
* Remove mixedCase variable and function names
pgkirsch added a commit that referenced this issue Jul 9, 2021
* Move xfoil_wrapper.py to xfoil/wrapper.py
* Move XFoilFit class into xfoil/constraint_set.py
pgkirsch added a commit that referenced this issue Jul 9, 2021
* bverbose removed
* alpha0 as kwarg
* mixedcase var
* comments and whitespace
pgkirsch added a commit that referenced this issue Jul 10, 2021
* Added ex 6.3 to docs/source/examples/
* Deleted gpfit/examples/ dir
* Consolidated ex 6.3 into t_examples (deleted t_ex6_3.py)
pgkirsch added a commit that referenced this issue Jul 12, 2021
@pgkirsch
Copy link
Contributor Author

@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
Copy link
Contributor

bqpd commented Jul 12, 2021

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
Copy link
Contributor

bqpd commented Jul 12, 2021

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

pgkirsch added a commit that referenced this issue Jul 12, 2021
* Docstrings
* Whitespace (fewer blank lines in functions)
* Removed some cruft comments
pgkirsch added a commit that referenced this issue Jul 12, 2021
* Better citation
* Remove Getting Started (for now)
* Add another example
* Update conf
@pgkirsch pgkirsch mentioned this issue Jul 13, 2021
Merged
pgkirsch added a commit that referenced this issue Jul 13, 2021
* Removed spaces around *, /, **
* Fixed some of the more egregious line break situations
* Removed a stub unit test
@pgkirsch pgkirsch added this to the v0.1 milestone Jul 13, 2021
pgkirsch added a commit that referenced this issue Jul 14, 2021
* Use of solution warnings API for out-of-bounds warnings
* Use of NamedVariables environment
* Unit tests for FitConstraintSet
pgkirsch added a commit that referenced this issue Jul 15, 2021
pgkirsch added a commit that referenced this issue Jul 15, 2021
pgkirsch added a commit that referenced this issue Jul 15, 2021
@whoburg
Copy link
Collaborator

whoburg commented Aug 8, 2021

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
Copy link
Contributor Author

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 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants