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.

Sign up for GitHub

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

Jump to bottom

333 pre calculated links #41

Merged
merged 6 commits into from
Jul 4, 2024
Merged

333 pre calculated links #41

merged 6 commits into from
Jul 4, 2024

Conversation

ldavies99
Copy link
Collaborator

Summary

Added parameter to the ratio_of_means() function which the user can use to select pre-calculated link columns. This will then skip imputation of the links in the pipeline.

Checklists

This pull request meets the following requirements:

  • installable with all dependencies recorded
  • runs without error
  • follows PEP8 and project specific conventions
  • appropriate use of comments, for example no descriptive comments
  • functions documented using Numpy style docstings
  • assumptions and decisions log considered and updated if appropriate
  • unit tests have been updated to cover essential functionality for a reasonable range of inputs and conditions
  • other forms of testing such as end-to-end and user-interface testing have been considered and updated as required
  • tests suite passes (locally as a minimum)
  • peer reviewed with review recorded

@ldavies99 ldavies99 added the enhancement New feature or request label Jul 3, 2024
@ldavies99 ldavies99 self-assigned this Jul 3, 2024
Jday7879
Jday7879 requested changes Jul 4, 2024
View reviewed changes
Copy link
Collaborator

@Jday7879 Jday7879 left a comment

Choose a reason for hiding this comment

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

Code looks great nice job Luke. I've left a minor suggestion to swap the logic for the test case. After that's been done, or if you feel its better the way it currently is, I'm happy for it to be merged into main

mbs_results/ratio_of_means.py Outdated Show resolved Hide resolved
tests/test_ratio_of_means.py Outdated Show resolved Hide resolved
@ldavies99 ldavies99 requested a review from Jday7879 July 4, 2024 13:57
Jday7879
Jday7879 approved these changes Jul 4, 2024
View reviewed changes
Copy link
Collaborator

@Jday7879 Jday7879 left a comment

Choose a reason for hiding this comment

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

Looks great, happy for this to be merged into main :)

@ldavies99 ldavies99 merged commit 7139506 into main Jul 4, 2024
3 checks passed
@ldavies99 ldavies99 deleted the 333-pre-calculated-links branch July 4, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jday7879 Jday7879 Jday7879 approved these changes

Assignees

@ldavies99 ldavies99

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ldavies99 @Jday7879