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

Rename indices to mask. Closes issue #91 #92

Merged
merged 6 commits into from
Sep 4, 2024

Conversation

kyracho
Copy link
Contributor

@kyracho kyracho commented Sep 4, 2024

Checklist

Hi, this PR changes _treatment_variants_indices to _treatment_variants_indices in

  • metalearner.py
  • xlearner.py
  • tlearner.py
  • drlearner.py
  • test_learner.py

Closes issue #91

  • Added a CHANGELOG.rst entry

@kyracho kyracho requested a review from kklein as a code owner September 4, 2024 00:53
@kyracho kyracho force-pushed the rename_indices_mask branch from dae28f2 to eb47b2d Compare September 4, 2024 02:10
@kyracho
Copy link
Contributor Author

kyracho commented Sep 4, 2024

Hello, I apologize for the multiple commits and any confusion this may have caused. I was working through some issues and ended up with a bit of a messy commit history. I made the mistake of forgetting to build the documentation locally, and it took several commits to rename all occurrences of the internal variable. The force-push was to amend a typo in the last commit message... embarrassing! I appreciate your understanding, and I’ll be more mindful in the future to keep things cleaner and more concise.

@kklein kklein linked an issue Sep 4, 2024 that may be closed by this pull request
@kklein
Copy link
Collaborator

kklein commented Sep 4, 2024

Hi Kyra - thanks a lot for your contribution - this looks great! :)

I'd have two small asks:

  • Would you mind removing the changelog entry for 0.12.0? Since your change exclusively affects private, i.e. with a _ prefix, fields, we wouldn't consider it an user-facing change. We generally only try to document user-facing changes in the changelog. Apologies that this wasn't clearer - I'll update the PR template.
  • Would you mind removing the files you checked in to git in docs/api? These are usually auto-generated and not version-controlled by us.
    Screenshot 2024-09-04 at 2 25 18 PM

@kyracho
Copy link
Contributor Author

kyracho commented Sep 4, 2024

Hi kklein, no problem at all regarding the changes—I'll go ahead and remove the changelog entry for version 0.12.0 as well as the files checked into docs/api. Thank you for clarifying.

Copy link
Collaborator

@kklein kklein 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 a bunch! :)

@kklein kklein merged commit 93ec745 into Quantco:main Sep 4, 2024
12 checks passed
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.

Rename internal indices to mask
2 participants