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

add some extra rcparams #1611

Merged
merged 18 commits into from
Mar 26, 2021
Merged

add some extra rcparams #1611

merged 18 commits into from
Mar 26, 2021

Conversation

OriolAbril
Copy link
Member

@OriolAbril OriolAbril commented Mar 11, 2021

Description

Adds two new rcParams, data.log_likelihood and stats.ic_compare_method. Also fixes a couple of
mishaps in index_origin usage.

Checklist

  • Does the PR follow official
    PR format?
  • Is the code style correct (follows pylint and black guidelines)?
  • Is the new feature listed in the New features
    section of the changelog?

@ahartikainen
Copy link
Contributor

Logic is now wrong.

I have a model with a variable log_lik and also the correct log likelihood as log_lik2.

from_xyz(log_likelihood="log_lik2") --> log_lik goes to log_likelihood.

@OriolAbril
Copy link
Member Author

True, it only has to be triggered if log_likelihood is exactly true, not if it evaluates to true, I always mess this up

@OriolAbril OriolAbril force-pushed the rcparams branch 4 times, most recently from eb25348 to 686dad7 Compare March 26, 2021 00:58
@ahartikainen
Copy link
Contributor

ahartikainen commented Mar 26, 2021

I wonder if there was something in the main that could fix that weird error.

@ahartikainen
Copy link
Contributor

Yeah, when there is true (?) and no log_lik, it will of course fail, so we need to put it back to None.

@OriolAbril
Copy link
Member Author

I was thinking that it may be easier to set self.log_likelihood = None and then start evaluating conditions on rcparams and log_likelihood argument to see if it needs to be updated and how. At least I don't see the current logic very clear which ends up in messing up. I'll give it a go later this evening

@OriolAbril
Copy link
Member Author

nevermind, it's basically the same, it would be something like:

self.log_likelihood = None
log_likelihood = rcParams if ...  # instead of doing that on self.log_likelihood
# [...]
# keep the same conditions but using log_likelihood instead of self.log_likelihood
# skip the if isinstance(..., bool)

which is not any improvement. Let's merge like this

@OriolAbril OriolAbril merged commit 23e14fb into main Mar 26, 2021
@OriolAbril OriolAbril deleted the rcparams branch March 26, 2021 21:09
utkarsh-maheshwari pushed a commit to utkarsh-maheshwari/arviz that referenced this pull request May 27, 2021
* add some extra rcparams

* black+changelog

* fix imports

* fix logic

* fix tests

* Fix log_likelihood behaviour

* add more tests

* fix typo

* fix lint

* black

* mypy fix

* add plot.density_type

* black

* update changelog

* update mypy.ini

* fix tests

* update empty test

* fix logic again

Co-authored-by: Ari Hartikainen <ahartikainen@users.noreply.github.com>
Co-authored-by: Ari Hartikainen <hartikainen.ari@gmail.com>
Co-authored-by: Ari Hartikainen <ari.hartikainen@ramboll.fi>
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.

2 participants