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

Remove notebooks, replace with pymc-examples submodule #4348

Merged
merged 15 commits into from
Dec 19, 2020

Conversation

MarcoGorelli
Copy link
Contributor

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

docs/source/index.rst Outdated Show resolved Hide resolved
Comment on lines 141 to 143
os.path.join(
os.path.dirname(os.path.dirname(srcdir)), "docs", "source", "pymc-examples", "notebooks"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now I've put the submodule in docs/source, later it could be taken out with (Michael had mentioned nbsphinx_link)

@MarcoGorelli
Copy link
Contributor Author

MarcoGorelli commented Dec 17, 2020

I still can't see anything from the 'tutorials' and 'examples' tabs, but I can't see anything there when I build the docs from master either - @ColCarroll is there a way to check that they're correct?

The rest (e.g. the quickstart button linking to the API quickstart) works fine

EDIT

Sorry Colin, just saw your reply about this - will try again and'll let you know!

@codecov
Copy link

codecov bot commented Dec 17, 2020

Codecov Report

Merging #4348 (1e8b9ea) into master (b7b145d) will increase coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4348      +/-   ##
==========================================
+ Coverage   87.85%   87.95%   +0.09%     
==========================================
  Files          88       88              
  Lines       14495    14499       +4     
==========================================
+ Hits        12734    12752      +18     
+ Misses       1761     1747      -14     
Impacted Files Coverage Δ
pymc3/plots/posteriorplot.py 95.65% <0.00%> (+74.59%) ⬆️

@MarcoGorelli
Copy link
Contributor Author

MarcoGorelli commented Dec 17, 2020

@ColCarroll works now! A few screenshots:

image


After clicking "quickstart":
image


After clicking "tutorials":
image


Clicking "using shared variables":
image


Clicking 'variational inference' from the homepage:
image

@twiecki
Copy link
Member

twiecki commented Dec 17, 2020

LGTM, definitely release-notes worthy.

@fonnesbeck
Copy link
Member

We should probably move the python script examples out, as well as the notebooks.

@MarcoGorelli
Copy link
Contributor Author

Sure, done here and in pymc-devs/pymc-examples#5

@MarcoGorelli
Copy link
Contributor Author

@fonnesbeck @twiecki what about examples/data, which is used by pm.get_data? Unless there's a plan to deprecate, I think that would need to stay here (though we could move just the examples scripts)

@fonnesbeck
Copy link
Member

fonnesbeck commented Dec 17, 2020

I think it makes sense to keep all the examples together. If the examples get imported as a submodule by default, nothing should break, should it? We can add an informative error message if pymc-examples is for some reason not installed.

@MarcoGorelli
Copy link
Contributor Author

If the examples get imported as a submodule by default

Not sure what you mean here - the submodule is available if you clone the repo and then do git submodule update --init --recursive, but I don't think it would be part of the PyPI package

Anyway, that feels like a separate discussion - I've reverted changes to pymc3/examples, they can be moved in a separate PR if desired, this one will only touch the notebooks

@MarcoGorelli
Copy link
Contributor Author

MarcoGorelli commented Dec 17, 2020

Anyway, that feels like a separate discussion - I've reverted changes to pymc3/examples, they can be moved in a separate PR if desired, this one will only touch the notebooks

Hey - re-reading this, I think I may have come across as rude - sorry about this, please allow me to rephrase:

I think that moving notebooks out of PyMC3 and putting pymc-examples as a git submodule in their place allows for the docs to keep being built relatively seamlessly. Furthermore, it doesn't change packaging because docs/source/notebooks are already kept out of the PyPI package.

On the other hand, pymc3/examples are currently part of the PyPI package, and so moving them out requires a bit more thought and discussion. Hence, I'd rather keep that to a separate PR so as to not delay moving the notebooks out for too long as well as too not make this PR even bigger. However, I'm happy to address pymc/examples here as well if you prefer

@fonnesbeck
Copy link
Member

That's fine, it can be a separate PR, as long as they eventually move over. I don't think the .py scripts in the examples folder get used very much, so its unlikely to be disruptive. The location of the associated data is something to discuss, however, as some of the datasets are used in tests.

@MarcoGorelli
Copy link
Contributor Author

Sure, thanks - I've opened #4352 for moving out pymc3/examples. The data used by tests would go in pymc3/tests/data, the rest would be downloaded from the pymc-examples repo

@fonnesbeck
Copy link
Member

That's probably a reasonable solution. Thanks.

@MarcoGorelli MarcoGorelli marked this pull request as draft December 19, 2020 09:13
@MarcoGorelli
Copy link
Contributor Author

MarcoGorelli commented Dec 19, 2020

Marking as draft because this breaks with the latest reorganisations in pymc-examples, will get back to it

@MarcoGorelli
Copy link
Contributor Author

MarcoGorelli commented Dec 19, 2020

works again 🎉

image

NOTE

This requires pymc-devs/pymc-examples#10 to be merged in order to work if you want to try building the docs locally

@MarcoGorelli MarcoGorelli marked this pull request as ready for review December 19, 2020 09:29
Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @MarcoGorelli !

@AlexAndorra
Copy link
Contributor

BTW, can I merge or do we wait for Colin's review?

@twiecki
Copy link
Member

twiecki commented Dec 19, 2020

@AlexAndorra Merge away.

@AlexAndorra AlexAndorra merged commit 34447a7 into pymc-devs:master Dec 19, 2020
@MarcoGorelli MarcoGorelli deleted the remove-notebooks branch December 19, 2020 11:38
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.

4 participants