Bring back logps #6272

ricardoV94 · 2022-11-07T15:46:16Z

This PR brings back univariate continuous logp methods that had been moved to Aeppl

~~I also removed all old logp/logcdf docstrings. They were both wrong and too cumbersome to update.~~ Will do in a separate PR

Closes #5329

Major / Breaking Changes

...

Bugfixes / New features

...

Docs / Maintenance

Remove old logp and logcdf docstrings

codecov · 2022-11-07T16:38:03Z

Codecov Report

Merging #6272 (b95d30c) into main (cb37afa) will increase coverage by 3.86%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6272      +/-   ##
==========================================
+ Coverage   90.39%   94.25%   +3.86%     
==========================================
  Files         111      111              
  Lines       23869    23930      +61     
==========================================
+ Hits        21577    22556     +979     
+ Misses       2292     1374     -918

Impacted Files	Coverage Δ
pymc/distributions/continuous.py	`97.84% <100.00%> (+0.33%)`	⬆️
pymc/distributions/discrete.py	`99.22% <100.00%> (ø)`
pymc/distributions/multivariate.py	`92.28% <100.00%> (ø)`
pymc/tests/distributions/test_continuous.py	`99.76% <100.00%> (+<0.01%)`	⬆️
pymc/aesaraf.py	`94.62% <0.00%> (-0.25%)`	⬇️
pymc/distributions/shape_utils.py	`97.87% <0.00%> (+0.42%)`	⬆️
pymc/sampling/parallel.py	`88.81% <0.00%> (+1.04%)`	⬆️
pymc/distributions/logprob.py	`67.28% <0.00%> (+5.55%)`	⬆️
pymc/step_methods/hmc/quadpotential.py	`80.62% <0.00%> (+6.77%)`	⬆️
pymc/model_graph.py	`78.82% <0.00%> (+12.35%)`	⬆️
... and 3 more

rlouf · 2022-11-08T14:32:07Z

I am curious to see how you are going to do attribution here since you're breaking Git history. In AePPL history this seems to come from this commit by Brandon, who is not a PyMC member.

ricardoV94 · 2022-11-08T14:40:01Z

I am curious to see how you are going to do attribution here since you're breaking Git history. In AePPL history this seems to come from this commit by Brandon, who is not a PyMC member.

Good question. In this specific case I don't see any issues. There was a previous PR that removed all the logp methods involved in this PR towards Aeppl, and no questions of attribution or git history were raised then: #4887

For future PRs, do you have any suggestion? A non-inconsequent portion of the Aeppl code was originally developed in PyMC and moved to Aeppl for unclear reasons.

ricardoV94 · 2022-11-08T14:43:38Z

pymc/distributions/continuous.py

@@ -1898,19 +1714,6 @@ def moment(rv, size, nu, mu, sigma):
        return mu

    def logp(value, nu, mu, sigma):
-        """


Because they were wrong since V4 began, and it's not supposed to be a user-facing method anyway

canyon289 · 2022-11-08T14:44:34Z

@ricardoV94 git is being a bit flaky for me, i understand why the comments are removed! you had mentioned it at in the PR body which I missed my apologies!

brandonwillard · 2022-11-08T14:58:11Z

Interesting that you've unnecessarily decided on an approach to all this that removes the original Git history of the material you're taking, especially since you've already forked AePPL itself. Was that intended or do you have some sort of excuse for doing things this way?

Good question. In this specific case I don't see any issues. There was a previous PR that removed all the logp methods involved in this PR towards Aeppl, and no questions of attribution or git history were raised then: #4887

For future PRs, do you have any suggestion? A non-inconsequent portion of the Aeppl code was originally developed in PyMC and moved to Aeppl for unclear reasons.

Wow, that's a bold and curiously aggressive and unbacked claim with some very overtly negative implications about me and everyone else who worked on that material across both AePPL and PyMC. Please, demonstrate to us exactly where attribution in AePPL was appropriate and missing. Don't forget to check who originally wrote the code that was moved!

canyon289

My 2 cents readd the docstrings from dropped in d34b557b5e0a0806842e2d4ec5aa5a842dbf8dfe for two reasons

Even though theyre stale they contain some useful information such as on here. https://github.com/pymc-devs/pymc/pull/6272/files#diff-9ac5fc2380425e161c832a41b1a173cac7693410b99e9c746d39507d96a8d357L1024-L1042
To update them its a great issue for beginners, and its simpler for beginners to start with an existing docstring and modify than start one from scratch

Per slack convo

Keeping them is bad because they're stale
When new logps get added they wont have docstrings so things will be inconsistent
We could autogenerate them

One question about autogeneration in this example here for instance, how would an autogenerated docstring tell us what the parameters good and bad mean? If I was a developer working on this in the future where would I get this information of what these mean?

https://github.com/pymc-devs/pymc/pull/6272/files#diff-9ac5fc2380425e161c832a41b1a173cac7693410b99e9c746d39507d96a8d357L1025-L1042

canyon289 · 2022-11-08T15:17:03Z

@brandonwillard @rlouf in all sincerity I appreciate you bringing up these concerns. I think there's low probability that any sort of understanding will be reached on this PR. Given that is another mode of discussion possible? Happy with whatever your suggestion is.

twiecki · 2022-11-08T15:50:54Z

@brandonwillard @rlouf How about we add you as co-authors on the commits?

rlouf · 2022-11-08T16:00:42Z

@twiecki I appreciate the offer, but for one I wouldn't want to be credited for a piece of code I didn't write. I looked around and found this answer very helpful: https://opensource.stackexchange.com/questions/11631/how-to-properly-attribute-others-people-work-on-github

twiecki · 2022-11-08T16:07:04Z

@rlouf Thanks, that's helpful. Yeah, let's find out the best way of including that in the copyright and license information. There are already some similar references there, e.g. https://github.com/aesara-devs/aesara/blob/main/doc/LICENSE.txt#L11

@brandonwillard Let me know if you want to be co-authored on the commits in addition to the above, or what you think the appropriate way of attributioning is.

rlouf · 2022-11-08T18:32:03Z

@rlouf Thanks, that's helpful. Yeah, let's find out the best way of including that in the copyright and license information. There are already some similar references there, e.g. https://github.com/aesara-devs/aesara/blob/main/doc/LICENSE.txt#L11

Building upon other people's work instead of copy/pasting avoids all this hassle. The full git history is kept and attribution is simple. The file you're pointing to is an excellent example: https://github.com/aesara-devs/aesara/commits/main/doc/LICENSE.txt

twiecki · 2022-11-09T13:51:10Z

LICENSE

-Copyright (c) 2009-2017 The PyMC developers (see contributors to pymc-devs on GitHub)
+Copyright (c) 2009-2022 The PyMC developers (see contributors to pymc-devs on GitHub)
+
+Contains code from Aeppl, Copyright (c) 2021-2022, Aesara Developers


@rlouf @brandonwillard Are you happy with this?

OriolAbril · 2022-11-09T15:13:42Z

LICENSE

@@ -6,7 +6,10 @@ PyMC is distributed under the Apache License, Version 2.0

 Copyright (c) 2006 Christopher J. Fonnesbeck (Academic Free License)
 Copyright (c) 2007-2008 Christopher J. Fonnesbeck, Anand Prabhakar Patil, David Huard (Academic Free License)
-Copyright (c) 2009-2017 The PyMC developers (see contributors to pymc-devs on GitHub)
+Copyright (c) 2009-2022 The PyMC developers (see contributors to pymc-devs on GitHub)


while we are editing this, this copyright sentence should not have end date

How should it be?

Copyright (c) 2009 The PyMC developers (see contributors to pymc-devs on GitHub)

Copyright (c) 2009- The PyMC developers (see contributors to pymc-devs on GitHub)

Copyright (c) 2009-present The PyMC developers (see contributors to pymc-devs on GitHub)

Or something else?

I like the 2009-present version most

@ricardoV94 @OriolAbril, let's fix the dates later, because then "while we're at it" we can update it in all places

for all intents and purposes of this PR it's ready to merge now that the attribution part has been taken care of

michaelosthege · 2022-11-09T20:34:44Z

LICENSE

@@ -6,7 +6,10 @@ PyMC is distributed under the Apache License, Version 2.0

 Copyright (c) 2006 Christopher J. Fonnesbeck (Academic Free License)
 Copyright (c) 2007-2008 Christopher J. Fonnesbeck, Anand Prabhakar Patil, David Huard (Academic Free License)
-Copyright (c) 2009-2017 The PyMC developers (see contributors to pymc-devs on GitHub)
+Copyright (c) 2009-2022 The PyMC developers (see contributors to pymc-devs on GitHub)


@ricardoV94 @OriolAbril, let's fix the dates later, because then "while we're at it" we can update it in all places

for all intents and purposes of this PR it's ready to merge now that the attribution part has been taken care of

Also add parameter check to uniform logcdf

ricardoV94 added docs logprob labels Nov 7, 2022

ricardoV94 requested review from twiecki, canyon289 and OriolAbril November 7, 2022 15:46

ricardoV94 force-pushed the bring_back_logp branch 3 times, most recently from fd9a6f5 to aefb017 Compare November 7, 2022 16:33

ricardoV94 force-pushed the bring_back_logp branch 2 times, most recently from 2c83bcc to db4f688 Compare November 8, 2022 10:15

ricardoV94 commented Nov 8, 2022

View reviewed changes

canyon289 reviewed Nov 8, 2022

View reviewed changes

ricardoV94 force-pushed the bring_back_logp branch from db4f688 to 9dd4ef6 Compare November 9, 2022 08:09

ricardoV94 mentioned this pull request Nov 9, 2022

Remove wrong type-hints and stale docstrings from distributions #6280

Merged

ricardoV94 force-pushed the bring_back_logp branch 2 times, most recently from 6a2dc2d to 4a476d9 Compare November 9, 2022 12:46

twiecki reviewed Nov 9, 2022

View reviewed changes

OriolAbril reviewed Nov 9, 2022

View reviewed changes

michaelosthege approved these changes Nov 9, 2022

View reviewed changes

Remove unused classmethod

79f590b

ricardoV94 added 3 commits November 20, 2022 10:13

Standardize check_parameters comparison order with message

42fd461

Bring back univariate continuous logp methods

cf13e77

Also add parameter check to uniform logcdf

Update LICENSE

b95d30c

ricardoV94 force-pushed the bring_back_logp branch from 4a476d9 to b95d30c Compare November 20, 2022 09:18

ricardoV94 merged commit c4a79ef into pymc-devs:main Nov 20, 2022

ricardoV94 mentioned this pull request Nov 20, 2022

Standardize test messages to use the permissible constraints (i.e. the thing being asserted) #6158

Closed

ricardoV94 deleted the bring_back_logp branch February 2, 2023 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring back logps #6272

Bring back logps #6272

ricardoV94 commented Nov 7, 2022 •

edited

Loading

codecov bot commented Nov 7, 2022 •

edited

Loading

rlouf commented Nov 8, 2022

ricardoV94 commented Nov 8, 2022

ricardoV94 Nov 8, 2022 •

edited

Loading

canyon289 commented Nov 8, 2022

brandonwillard commented Nov 8, 2022 •

edited

Loading

canyon289 left a comment

canyon289 commented Nov 8, 2022

twiecki commented Nov 8, 2022

rlouf commented Nov 8, 2022

twiecki commented Nov 8, 2022

rlouf commented Nov 8, 2022

twiecki Nov 9, 2022

OriolAbril Nov 9, 2022

ricardoV94 Nov 9, 2022 •

edited

Loading

michaelosthege Nov 9, 2022

michaelosthege Nov 9, 2022 •

edited

Loading

michaelosthege Nov 9, 2022 •

edited

Loading

Bring back logps #6272

Bring back logps #6272

Conversation

ricardoV94 commented Nov 7, 2022 • edited Loading

Major / Breaking Changes

Bugfixes / New features

Docs / Maintenance

codecov bot commented Nov 7, 2022 • edited Loading

Codecov Report

rlouf commented Nov 8, 2022

ricardoV94 commented Nov 8, 2022

ricardoV94 Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

canyon289 commented Nov 8, 2022

brandonwillard commented Nov 8, 2022 • edited Loading

canyon289 left a comment

Choose a reason for hiding this comment

canyon289 commented Nov 8, 2022

twiecki commented Nov 8, 2022

rlouf commented Nov 8, 2022

twiecki commented Nov 8, 2022

rlouf commented Nov 8, 2022

twiecki Nov 9, 2022

Choose a reason for hiding this comment

OriolAbril Nov 9, 2022

Choose a reason for hiding this comment

ricardoV94 Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

michaelosthege Nov 9, 2022

Choose a reason for hiding this comment

michaelosthege Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

michaelosthege Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

ricardoV94 commented Nov 7, 2022 •

edited

Loading

codecov bot commented Nov 7, 2022 •

edited

Loading

ricardoV94 Nov 8, 2022 •

edited

Loading

brandonwillard commented Nov 8, 2022 •

edited

Loading

ricardoV94 Nov 9, 2022 •

edited

Loading

michaelosthege Nov 9, 2022 •

edited

Loading

michaelosthege Nov 9, 2022 •

edited

Loading