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

Item response jupyter style update #309

Merged

Conversation

ltoniazzi
Copy link
Member

@ltoniazzi ltoniazzi commented Apr 10, 2022

Updated the Item Response NBA notebook to follow the Jupyter style guidelines. Issue #217

Made modifications to adhere to the Jupyter style guidelines.

Note: I failed to resolve this warning after sampling:

UserWarning: The parameter 'updates' of aesara.function() expects an OrderedDict, got <class 'dict'>. 
Using a standard dictionary here results in non-deterministic behavior. 
You should use an OrderedDict if you are using Python 2.7 (collections.OrderedDict for older python), 
or use a list of (shared, update) pairs. Do not just convert your dictionary to this 
type before the call as the conversion will still be non-deterministic.")

Helpful links

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-04-10T20:29:00Z
----------------------------------------------------------------

Something went wrong with the tags. And the date should be updated so it appears in the "recent updates" section


@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-04-10T20:29:01Z
----------------------------------------------------------------

I think the graphviz won't appear if the semicolon is used.


@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-04-10T20:29:01Z
----------------------------------------------------------------

You should be able to use labeller=az.labels.NoVarLabeller() when calling the plotting function for the theta in the first row and the [] to not appear in the tick labels. I say should because the labels module is still untested, if it doesn't we'll fix whatever issue appears


ltoniazzi commented on 2022-04-12T21:58:00Z
----------------------------------------------------------------

Tried two commands but they do not seem to get rid of the brakets. You can see them in the last commit.

@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-04-10T20:29:02Z
----------------------------------------------------------------

Line #2.    top_theta_list = top_theta[:amount].coords["disadvantaged"].to_dict()["data"]

I think this is overly complicated. In general, positional indexing should not be used in xarray, because the order of the dimensions has no guarantees of being always the same. Here however the variable is already 1d because we reduced the chain and dim ones, so it is perfectly fine to use positional indexing. The selection however can be simplified to

top_theta_players = top_theta["disadvantaged"][:amount]        # to get xr.DataArray

top_theta_players = top_theta["disadvantaged"][:amount].values # to get numpy array

also, these variables with the top players, should be used further down in the plot instead of repeating the selection again.

I should have commented that on the cell above, I somehow messed it up. It applies to both, and once defined, it the variable can also be used without needing to be redefined.



@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-04-10T20:29:03Z
----------------------------------------------------------------

I did not reexecute the notebook and should not be listed here, my PR was only generating the myst views because they are helpful for reviewing but I had no real effect on the actual notebooks other than cleaning up trailing white spaces.

We also agreed that only people who authored, adapted or updated the notebook would be listed in the post directive even if everyone is listed here. The metadata from the post directive is what is used to generate the citation advise below


Copy link
Member Author

Tried two commands but they do not seem to get rid of the brakets. You can see them in the last commit.


View entire conversation on ReviewNB

ricardoV94 and others added 13 commits April 14, 2022 22:30
* Add guide on how to wrap a JAX function in a Aesara Op

* Fix typos and reorder last sections

* Fix reference paths

* Simplify model and be more verbose in Op creation

* Address review comments

* Address more review comments

* Fix more Aesara docs references

* Use reference to Aesara index page

* Update myst_nbs/case_studies/wrapping_jax_function.myst.md

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
…evs#310)

* create truncated regression example

* delete truncated regression example from main branch

* create truncated regression example

* delete truncated regression example from main branch

* create truncated regression example

* delete truncated regression example from main branch

* initial commit

* update link to pull request in Authors section

* add tag `classification`

* update authorship verbs

* plot through xarray, using XrContinuousRV

* add x axis labels

* add xarray_einstats to watermark, and fix 'classification' as a tag, not a category

Co-authored-by: Benjamin T. Vincent <drbenvincent@users.noreply.github.com>
Add *.DS_Store to .gitignore

Co-authored-by: Benjamin T. Vincent <drbenvincent@users.noreply.github.com>
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 22, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-04-22T21:12:38Z
----------------------------------------------------------------

Line #2.    top_theta_players = top_theta["disadvantaged"][:amount].to_dict()["data"]

What is the .to_dict()["data"] doing? I don't know what is going on and have never used that myself, but I am also positive whatever is being achieved here can be done in a simpler way. If the goal is to convert the DataArray to a numpy array, using .values does the trick for example.


ltoniazzi commented on 2022-04-23T11:52:53Z
----------------------------------------------------------------

Sorry this was a mistake that popped up when I restored the notebook

Copy link
Member Author

Sorry this was a mistake that popped up when I restored the noebook


View entire conversation on ReviewNB

@OriolAbril OriolAbril merged commit 880e273 into pymc-devs:main Apr 24, 2022
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