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

Implement cloneable interface in ndarrays #13232

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Jun 25, 2023

fixes #13226

@mattpap mattpap added this to the 3.3 milestone Jun 25, 2023
@codecov
Copy link

codecov bot commented Jun 25, 2023

Codecov Report

Merging #13232 (1f5f145) into branch-3.3 (c199eda) will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           branch-3.3   #13232   +/-   ##
===========================================
  Coverage       92.43%   92.43%           
===========================================
  Files             315      315           
  Lines           20097    20097           
===========================================
  Hits            18577    18577           
  Misses           1520     1520           

@bryevdv
Copy link
Member

bryevdv commented Jun 27, 2023

Are users expected to use Cloner directly? If that's the case we would at least need an example that demonstrates this usage, otherwise there is no way users would know about it. I guess I was expecting a .clone() method (which would still need to be demonstrated explicitly)

@mattpap
Copy link
Contributor Author

mattpap commented Jul 18, 2023

I don't expect many users to currently use this at all. This is an internal API right now, though users can import and use it. So currently it is the expectation to use Cloner class. There is no .clone() method, because there is no need to make bokehjs' classes heavier than they already and also cloning is a very expensive operation, so forcing explicit usage may make the user of this API reflect whether this is what they really ought to do. Of course at some point it would be good to expose this as a part of a public API, under the broad umbrella of TS/JS API.

@bryevdv
Copy link
Member

bryevdv commented Jul 18, 2023

@mattpap I ask because the issue this PR claims to fix was an unfulfilled need brought by a user. What is the guidance to give to that user to solve their problem?

If this PR does not offer them a solution then it should not close the issue.

@mattpap
Copy link
Contributor Author

mattpap commented Jul 18, 2023

Contradicting my earlier statement, there's already HasProps.clone() method, which invokes Cloner class, so this PR ineed fixes the issue. I forgot this method actually exists.

@mattpap mattpap mentioned this pull request Jul 18, 2023
14 tasks
@bryevdv
Copy link
Member

bryevdv commented Jul 18, 2023

@mattpap 👍 if you can also give a line or two of specific guidance to the OP I am sure it would be appreciated

@mattpap mattpap merged commit 67385ce into branch-3.3 Jul 19, 2023
27 checks passed
@mattpap mattpap deleted the mattpap/13226_ndarray_clone branch July 19, 2023 17:28
@mattpap mattpap modified the milestones: 3.3, 3.2.1 Jul 19, 2023
mattpap added a commit that referenced this pull request Jul 20, 2023
* Improve performance of WebGL line glyph (#13236)

* Improve performance of webgl line glyph

* Update baseline images

* Update ruff repo links (#13242)

* Fix `PropertyValueColumnData._stream()` to handle `rollover=0` (#13239)

* Fix `PropertyValueColumnData._stream()` to handle `rollover=0`

* Check 'event.data' in the unit tests

* Improve docs example in first_steps_8.rst (#13161)

* Improve docs example in first_steps_8.rst

* Update figure in docs example first_steps_8.rst

* Resolve issues with high DPI GridPlot exports (#13253)

* Allow to disable DatePicker (etc.) after creation (#13256)

* Use ../core/kinds instead of core/kinds in imports (#13254)

* Don't paint undisplayed plots (#13250)

* Update theme.py (#13270)

* Fixed Broken Link (#13266)

* Fixed Broken Link

* Update docs/bokeh/source/docs/first_steps/first_steps_9.rst

Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>

---------

Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>

* fix unterminated string literals in example code [skip ci] (#13274)

* Fix Legend's grid layout for uneven number of items (#13263)

* 13272 update legend docs (#13273)

* update docs for two dimensional legends

* Apply suggestions from code review

* Implement cloneable interface in ndarrays (#13232)

* Update docs/bokeh/switcher.json

* Added release notes

---------

Co-authored-by: Ian Thomas <ianthomas23@gmail.com>
Co-authored-by: Xiaoyang Liu <siujoeng.lau@gmail.com>
Co-authored-by: Christoph Deil <Deil.Christoph@gmail.com>
Co-authored-by: Moritz Schreiber <68053396+mosc9575@users.noreply.github.com>
Co-authored-by: Rajat Shenoi <rajatshenoi@outlook.com>
Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] make bokehjs ndarrays cloneable to enable client-side image transforms
3 participants