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 warning for aliases athat aren't used #691

Merged
merged 4 commits into from
Jun 26, 2023
Merged

Add warning for aliases athat aren't used #691

merged 4 commits into from
Jun 26, 2023

Conversation

tjburch
Copy link
Contributor

@tjburch tjburch commented Jun 25, 2023

Resolves #675 by throwing a warning if any aliases are not used. I didn't want to make the warnings too verbose, so if there's 5 or fewer aliases that aren't used, it explicitly tells you which aren't used. If more than 5 aren't used, it throws a generic message.

bambi/models.py Outdated
@@ -517,6 +518,17 @@ def set_alias(self, aliases):
for term in self.response_component.group_specific_terms.values():
if name in term.prior.args:
term.hyperprior_alias = {name: alias}

if (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a comment to this one and the code block below so when reading this module its more clear how the list, and the if missing_name block below interact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! Tomas can comment on the rest of the logic. LGTM but he knows best!

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly this is "asking" the same questions that were in the for loop again. If they are all False, it means none of the if-blocks in the previous if-statements got executed, implying the alias was not used.

What do you think about this alternative approach (pseudo-code)?

for name, alias in aliases.items():
    is_used = False
    
    if cond1:
        ....
        is_used = True

    if cond2:
        ...
        is_used = True


     # finally
     if not is_used:
          missing_names.append(name)

I know it introduces a new auxiliary variable. But I think it's slightly clearer since the human-reader doesn't need to parse a "complex" if statement with multiple conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely cleaner. Added.

@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2023

Codecov Report

Merging #691 (03c74ff) into main (259c474) will increase coverage by 0.00%.
The diff coverage is 88.88%.

@@           Coverage Diff           @@
##             main     #691   +/-   ##
=======================================
  Coverage   88.32%   88.33%           
=======================================
  Files          40       40           
  Lines        2862     2880   +18     
=======================================
+ Hits         2528     2544   +16     
- Misses        334      336    +2     
Impacted Files Coverage Δ
bambi/models.py 79.16% <88.88%> (+0.51%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tomicapretto
Copy link
Collaborator

Thanks a lot again @tjburch for the useful contribution. And thanks @canyon289 for the review!

I think the logic is good. Still, I left a comment suggesting an approach that may be a little bit better. Let me know what you think.

The only thing I would add to the PR is a test that checks the warning is raised. It can go into tests/test_aliases.py

@tjburch
Copy link
Contributor Author

tjburch commented Jun 26, 2023

Thanks for the reviews. Made the specified changes, and just added the tests too.

@tomicapretto tomicapretto merged commit e1b0e8c into bambinos:main Jun 26, 2023
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.

Raise an exception (or warning) when key doesn't exist in set_alias
4 participants