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

Unify recursive update on ReplacementPane #4958

Merged
merged 8 commits into from May 30, 2023
Merged

Conversation

Hoxbro
Copy link
Member

@Hoxbro Hoxbro commented May 28, 2023

Fixes #4951
Fixes #4968

@codecov
Copy link

codecov bot commented May 28, 2023

Codecov Report

Merging #4958 (f97d028) into main (2ab34b3) will decrease coverage by 10.05%.
The diff coverage is 87.50%.

@@             Coverage Diff             @@
##             main    #4958       +/-   ##
===========================================
- Coverage   83.49%   73.44%   -10.05%     
===========================================
  Files         270      271        +1     
  Lines       38239    38281       +42     
===========================================
- Hits        31926    28116     -3810     
- Misses       6313    10165     +3852     
Flag Coverage Δ
ui-tests ?
unitexamples-tests 73.44% <87.50%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
panel/pane/base.py 86.17% <73.33%> (-0.92%) ⬇️
panel/tests/test_param.py 99.71% <100.00%> (+<0.01%) ⬆️

... and 57 files with indirect coverage changes

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

@philippjfr philippjfr changed the title Add _is_equal in ReplacementPane Unify recursive update on ReplacementPane May 28, 2023
@Hoxbro
Copy link
Member Author

Hoxbro commented May 28, 2023

I think it would be beneficial to have _generate_hash as a fallback and do the simple equally check first. Mainly because it _generate_hash is significantly slower for most cases.

Maybe even have a .all() try before using it.

@philippjfr
Copy link
Member

philippjfr commented May 28, 2023

My thinking was that in most cases the cost is neglible but in the case of a dataframe, where the cost does potentially matter significantly, simple equality won't, so you end up paying the cost twice.

@philippjfr
Copy link
Member

My thinking was that in most cases the cost is neglible but in the case of a dataframe where the cost does potentially matter significantly equal won't work anyway so you end up paying the cost twice.

The .all() could help with that. Not sure I prefer it though.

@Hoxbro
Copy link
Member Author

Hoxbro commented May 30, 2023

My thought was that we should not penalize every other type because Arrays and DataFrames are ambiguous with more than one element.

The .all is also faster than having to generate the hash.

@philippjfr
Copy link
Member

Fair points, swapped the order.

@philippjfr philippjfr merged commit aa9c6d7 into main May 30, 2023
12 of 15 checks passed
@philippjfr philippjfr deleted the improve_equal_check branch May 30, 2023 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants