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

Issue #400: Rename to naive model and latent model #433

Closed
wants to merge 15 commits into from
Closed

Conversation

athowes
Copy link
Collaborator

@athowes athowes commented Nov 14, 2024

Description

This PR closes #400. As agreed it replaces:

  • direct_model with naive_model
  • latent_individual with latent_model

I used find and replace to make sure there are no instances of "individual" or "direct" left.

This also needs me to make the change from "cohort" to "marginal" over at #221 which I'll do now.

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
  • I have read the contribution guidelines.
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • My code follows the established coding standards.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

@athowes athowes force-pushed the rename-models branch 2 times, most recently from bbff40e to 1fe2ce6 Compare November 14, 2024 15:31
@seabbs seabbs enabled auto-merge (squash) November 14, 2024 15:57
seabbs
seabbs previously approved these changes Nov 14, 2024
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.17%. Comparing base (d02c7c7) to head (bc00fd4).
Report is 50 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #433      +/-   ##
==========================================
+ Coverage   88.55%   94.17%   +5.61%     
==========================================
  Files          11       16       +5     
  Lines         402      429      +27     
==========================================
+ Hits          356      404      +48     
+ Misses         46       25      -21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seabbs
Copy link
Contributor

seabbs commented Nov 15, 2024

Sorry about this but duplicate work is in #441 so I think we should close this

@seabbs seabbs closed this Nov 15, 2024
auto-merge was automatically disabled November 15, 2024 15:08

Pull request was closed

@athowes
Copy link
Collaborator Author

athowes commented Nov 15, 2024

Hi @seabbs, I'm a bit confused/upset about you closing this.

We agreed about doing the work in this PR. This was already finished before #441. The work in #441 was not part of what we agreed to work on this week, and does not have an issue, or anyone else's input.

I am waiting on this name change implemented by #433 to rebase other PRs that we agreed to work on this week. So this not being merged is slowing me down on doing the work we need to do to finish up the existing issues we agreed to work on.

As general principle, PRs should be scoped out and modular rather than bundled in together. Work should be agreed upon and executed, rather than unilaterally decided. These are the (good) principles that you've taught me!

I'd prefer that we merged this existing PR. If there is duplicate work in #441 (as I say, which was done after this was already complete), then it'll be an easy rebase. I don't understand why not merge this and then rebase #441 with it merged in the first instance?

@athowes athowes reopened this Nov 15, 2024
@athowes athowes closed this Nov 15, 2024
@seabbs
Copy link
Contributor

seabbs commented Nov 15, 2024

Sorry for the crossed wires Adam and you are right it would be better to have PRs be based on scoped out issues.

@athowes athowes deleted the rename-models branch November 15, 2024 16:08
@athowes athowes restored the rename-models branch November 15, 2024 18:00
@athowes athowes reopened this Nov 15, 2024
@athowes
Copy link
Collaborator Author

athowes commented Nov 15, 2024

Too much work to rebase, closing again.

@athowes athowes closed this Nov 15, 2024
@seabbs seabbs deleted the rename-models branch November 29, 2024 11:17
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 latent_individual and direct_model for consistency
2 participants