Skip to content

Conversation

dfm
Copy link
Contributor

@dfm dfm commented Jun 7, 2022

Model.compile_fn requires all parameters except outs to be keyword arguments so the top level compile_fn didn't work as written.

Thank your for opening a PR!

Before you proceed, please make sure that the pre-commit linting/style checks pass.

Depending on what your PR does, here are a few things you might want to address in the description:

  • what are the (breaking) changes that this PR makes?
  • important background, or details about the implementation
  • are the changes—especially new features—covered by tests and docstrings?
  • consider adding/updating relevant example notebooks
  • right before it's ready to merge, mention the PR in the RELEASE-NOTES.md

`Model.compile_fn` requires all parameters except `outs` to be keyword arguments so the top level `compile_fn` didn't work as written.
@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #5865 (84dc7b4) into main (575454f) will decrease coverage by 3.70%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5865      +/-   ##
==========================================
- Coverage   89.48%   85.78%   -3.71%     
==========================================
  Files          73       73              
  Lines       13225    13225              
==========================================
- Hits        11835    11345     -490     
- Misses       1390     1880     +490     
Impacted Files Coverage Δ
pymc/model.py 76.42% <0.00%> (-10.85%) ⬇️
pymc/model_graph.py 17.60% <0.00%> (-76.00%) ⬇️
pymc/variational/updates.py 37.93% <0.00%> (-54.19%) ⬇️
pymc/distributions/timeseries.py 43.36% <0.00%> (-35.28%) ⬇️
pymc/data.py 70.20% <0.00%> (-11.43%) ⬇️
pymc/step_methods/hmc/quadpotential.py 73.56% <0.00%> (-6.99%) ⬇️
pymc/backends/arviz.py 85.42% <0.00%> (-4.86%) ⬇️
pymc/printing.py 21.90% <0.00%> (-4.77%) ⬇️
pymc/util.py 73.65% <0.00%> (-3.60%) ⬇️
... and 5 more

@twiecki
Copy link
Member

twiecki commented Jun 7, 2022

Maybe add a test?

@dfm
Copy link
Contributor Author

dfm commented Jun 7, 2022

This wasn't previously tested (hence why the bug existed!) so I'd say that that's beyond the scope of my PR, but if someone else wants to, that would be cool.

@ricardoV94
Copy link
Member

This wasn't previously tested (hence why the bug existed!) so I'd say that that's beyond the scope of my PR, but if someone else wants to, that would be cool.

Yeah we should test, so we weren't calling this function anywhere? It doesn't need to be you if you don't want to ofc. Thanks for opening the PR

ricardoV94
ricardoV94 previously approved these changes Jun 8, 2022
Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Thanks @dfm!

@dfm
Copy link
Contributor Author

dfm commented Jun 8, 2022

Wait until we see if the test executes - I just wrote it on the GitHub UI :D

test_vals = np.array([0.0, -1.0])
state = {"x": test_vals, "y_log__": test_vals}

for target in [x, y]:
Copy link
Member

@ricardoV94 ricardoV94 Jun 8, 2022

Choose a reason for hiding this comment

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

I don't think that should be the case :P (that it only accepts one output). Did we introduce that constraint somewhere by accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure - it's still not working with a strange error message!

@ricardoV94 ricardoV94 self-requested a review June 8, 2022 13:23
@dfm
Copy link
Contributor Author

dfm commented Jun 8, 2022

I'm not sure why my proposed test isn't working, but I'll close this PR and just open an issue and someone else can deal with it because I don't have time!

My proposed 5 character change fixes things in my case but YMMV :D

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.

3 participants