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

Run pyupgrade #4083

Merged
merged 3 commits into from
Sep 16, 2020
Merged

Run pyupgrade #4083

merged 3 commits into from
Sep 16, 2020

Conversation

aseyboldt
Copy link
Member

pyupgrade upgrades the syntax to make use of more recent python features. In our case those are mostly set literals and format strings. (https://github.com/asottile/pyupgrade)
It is a rather large diff, but I think it makes the source much nicer to read in a lot of places.

@codecov
Copy link

codecov bot commented Sep 6, 2020

Codecov Report

Merging #4083 into master will not change coverage.
The diff coverage is 72.36%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4083   +/-   ##
=======================================
  Coverage   88.76%   88.76%           
=======================================
  Files          89       89           
  Lines       14015    14015           
=======================================
  Hits        12440    12440           
  Misses       1575     1575           
Impacted Files Coverage Δ
pymc3/backends/hdf5.py 94.36% <ø> (ø)
pymc3/distributions/mixture.py 88.47% <ø> (ø)
pymc3/gp/util.py 80.00% <0.00%> (ø)
pymc3/math.py 68.47% <0.00%> (ø)
pymc3/ode/ode.py 93.75% <0.00%> (ø)
pymc3/plots/__init__.py 50.00% <0.00%> (ø)
pymc3/theanof.py 90.00% <0.00%> (ø)
pymc3/backends/base.py 87.30% <33.33%> (ø)
pymc3/backends/sqlite.py 93.54% <33.33%> (ø)
pymc3/distributions/continuous.py 92.93% <50.00%> (ø)
... and 19 more

Copy link
Member

@ColCarroll ColCarroll left a comment

Choose a reason for hiding this comment

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

I like this -- I took a brief look and it seems better than before. Is it possible to hold onto it, and rerun after #4076 gets merged? I think that would be a painful rebase, and if this is just a script, it might be easier for you to do.

@@ -339,8 +339,8 @@ def load(name, model=None):
db.connect()
varnames = _get_table_list(db.cursor)
if len(varnames) == 0:
raise ValueError(('Can not get variable list for database'
'`{}`'.format(name)))
raise ValueError('Can not get variable list for database'
Copy link
Member

Choose a reason for hiding this comment

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

Heh.

@@ -366,14 +366,14 @@ def _get_table_list(cursor):


def _get_var_strs(cursor, varname):
cursor.execute('SELECT * FROM [{}]'.format(varname))
cursor.execute(f'SELECT * FROM [{varname}]')
Copy link
Member

Choose a reason for hiding this comment

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

boy howdy we should get rid of this backend

Copy link
Member Author

Choose a reason for hiding this comment

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

yes...

``gaussian_kernel`` :math: `\sum \left(-0.5 \left(\frac{xo - xs}{\epsilon}\right)^2\right)`
``wasserstein`` :math: `\frac{1}{n} \sum{\left(\frac{|xo - xs|}{\epsilon}\right)}`
``energy`` :math: `\sqrt{2} \sqrt{\frac{1}{n} \sum \left(\frac{|xo - xs|}{\epsilon}\right)^2}`
``gaussian_kernel`` :math: `\\sum \\left(-0.5 \\left(\frac{xo - xs}{\\epsilon}\right)^2\right)`
Copy link
Member

Choose a reason for hiding this comment

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

well, this is wonderful

Copy link
Member Author

Choose a reason for hiding this comment

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

should be a raw string then...

@@ -584,7 +584,7 @@ def update(self, sample, grad, tune):

def raise_ok(self, vmap):
if self._chol_error is not None:
raise ValueError("{0}".format(self._chol_error))
raise ValueError(f"{self._chol_error}")
Copy link
Member

Choose a reason for hiding this comment

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

heh.

@aseyboldt
Copy link
Member Author

Waiting till after #4076.
It is trivial to recreate this PR

@MarcoGorelli
Copy link
Contributor

MarcoGorelli commented Sep 7, 2020

Can this also be added to the CI process to make sure that outdated syntax isn't used in the future?

EDIT

if #4088 is accepted it's probably easiest to add this to the pre-commit-config.yaml file

@AlexAndorra
Copy link
Contributor

+1 to add this to the pre-commit-config.yaml file as pre-commit hook -- will make our lives easier and our code consistently better!

@AlexAndorra
Copy link
Contributor

#4088 is now merged, so pyupgrade can be added as a pre-commit hook quite easily now 🎉

@twiecki
Copy link
Member

twiecki commented Sep 15, 2020

#4076 is merged so we can revisit this.

@aseyboldt aseyboldt force-pushed the pyupgrade branch 5 times, most recently from 1f76d50 to f8e3dc3 Compare September 16, 2020 11:23
@twiecki twiecki merged commit 67155fa into pymc-devs:master Sep 16, 2020
@twiecki
Copy link
Member

twiecki commented Sep 16, 2020

Thanks Dr Seyboldt!

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.

6 participants