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 default values using Pandas assign in design_matrix #8949

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

larsevj
Copy link
Contributor

@larsevj larsevj commented Oct 11, 2024

Resolve Pandas performance warning

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"')

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

@larsevj larsevj added the release-notes:unreleased-feature-changes PR with changes to a feature which is not yet released. Not for introduction of new features! label Oct 11, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.73%. Comparing base (90d11ec) to head (2a49444).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8949      +/-   ##
==========================================
- Coverage   90.76%   90.73%   -0.04%     
==========================================
  Files         351      351              
  Lines       21895    21893       -2     
==========================================
- Hits        19874    19864      -10     
- Misses       2021     2029       +8     
Flag Coverage Δ
cli-tests 39.23% <0.00%> (-0.02%) ⬇️
gui-tests 71.77% <0.00%> (+0.03%) ⬆️
performance-tests 49.41% <0.00%> (+0.04%) ⬆️
unit-tests 79.63% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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


Returns a dict of default values
Returns a dict of usable default values
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove the "usable" :)

@larsevj larsevj requested a review from xjules October 15, 2024 13:03
@larsevj larsevj self-assigned this Oct 24, 2024
@larsevj larsevj force-pushed the insert_new_columns_all_at_once branch from 383f774 to fe5cc7e Compare October 24, 2024 08:05
return {
row[0]: convert_to_numeric(row[1])
for _, row in default_df.iterrows()
if row[0] not in existing_parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we write a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

@larsevj larsevj force-pushed the insert_new_columns_all_at_once branch from fe5cc7e to 8dc3ba3 Compare October 31, 2024 15:00
@larsevj larsevj requested a review from xjules November 4, 2024 08:46
@larsevj larsevj force-pushed the insert_new_columns_all_at_once branch from 8dc3ba3 to 2a49444 Compare November 4, 2024 08:46
Copy link
Contributor

@xjules xjules left a comment

Choose a reason for hiding this comment

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

Nice! Just squash the commits and 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:unreleased-feature-changes PR with changes to a feature which is not yet released. Not for introduction of new features!
Projects
Status: Reviewed
Development

Successfully merging this pull request may close these issues.

3 participants