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

Fix shared array access on init #2697

Merged
merged 16 commits into from
Jun 3, 2022
Merged

Conversation

banesullivan
Copy link
Member

@banesullivan banesullivan commented May 25, 2022

Resolve #2696, depends on #2698, and generally improves how meshes are created without copying source data when possible

@MatthewFlamm, please review as this is a follow up to your #2070

I'd like to add some more/better tests for this scenario

Memory usage comparison

`main` This branch
 Line #    Mem usage    Increment  Occurrences   Line Contents
=============================================================
     6    138.4 MiB    138.4 MiB           1   @profile
     7                                         def test():
     8
     9   2427.2 MiB   2288.9 MiB           1       source = np.random.rand(100_000_000, 3)
    10
    11   2427.3 MiB      0.1 MiB           1       mesh = pv.PolyData()
    12   4716.2 MiB   2288.9 MiB           1       mesh.points = source
    13
    14   4716.2 MiB      0.0 MiB           1       pts = mesh.points
    15   4716.2 MiB      0.0 MiB           1       pts /= 2
Line #    Mem usage    Increment  Occurrences   Line Contents
=============================================================
     6    138.2 MiB    138.2 MiB           1   @profile
     7                                         def test():
     8
     9   2427.0 MiB   2288.9 MiB           1       source = np.random.rand(100_000_000, 3)
    10
    11   2427.1 MiB      0.1 MiB           1       mesh = pv.PolyData()
    12   2427.2 MiB      0.0 MiB           1       mesh.points = source
    13
    14   2427.2 MiB      0.0 MiB           1       pts = mesh.points
    15   2427.2 MiB      0.0 MiB           1       pts /= 2

@github-actions github-actions bot added the bug Uh-oh! Something isn't working as expected. label May 25, 2022
@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #2697 (e3bfa25) into main (975645d) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2697      +/-   ##
==========================================
- Coverage   93.82%   93.78%   -0.04%     
==========================================
  Files          75       76       +1     
  Lines       16148    16208      +60     
==========================================
+ Hits        15151    15201      +50     
- Misses        997     1007      +10     

banesullivan and others added 4 commits May 25, 2022 23:38
* Support no copy with StructuredGrid

* Support no copy with RectilinearGrid

* Improve convert_array to handle lists

* raise on duplication

* refactor

* add check_duplicates option

Co-authored-by: Alex Kaszynski <akascap@gmail.com>
@banesullivan banesullivan changed the title _coerce_pointslike_arg copy=False default Fix shared array access on init May 31, 2022
@banesullivan banesullivan marked this pull request as ready for review May 31, 2022 16:14
@banesullivan
Copy link
Member Author

I'm working to wrap up https://github.com/pyvista/pyvista-xarray and need this to merge and patch release as soon as we can

@akaszynski
Copy link
Member

I'm working to wrap up https://github.com/pyvista/pyvista-xarray and need this to merge and patch release as soon as we can

Per our agreement in our release discussion, we should give everyone 24 hours to review. However, I'm good to merge this sooner if critical.

pyvista/core/pointset.py Show resolved Hide resolved
pyvista/utilities/misc.py Outdated Show resolved Hide resolved
tests/utilities/test_common.py Outdated Show resolved Hide resolved
tests/test_create_no_copy.py Outdated Show resolved Hide resolved
tests/test_create_no_copy.py Outdated Show resolved Hide resolved
pyvista/core/grid.py Show resolved Hide resolved
@adeak
Copy link
Member

adeak commented May 31, 2022

My docstring changes seem to break docstring testing, but the failure seems to be in jupyter places(?) and I can't reproduce it locally.

Actually, things are falling apart everywhere in CI. What did I break?! 😄

tests/test_create_no_copy.py Outdated Show resolved Hide resolved
Copy link
Member

@adeak adeak left a comment

Choose a reason for hiding this comment

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

@akaszynski we're almost done here:

  1. One discussion about copy=False change, seems resolved to me.
  2. One discussion about fixing up StucturedGrid's __init__() and docs, for which I've opened Clean up StructuredGrid documentation and __init__() #2733
  3. Fix shared array access on init #2697 (comment) which needs input.

Approving anyway because 3. is still correct as is.

Co-authored-by: Andras Deak <adeak@users.noreply.github.com>
@akaszynski
Copy link
Member

  1. One discussion about copy=False change, seems resolved to me.

I think this is resolved as-is given @MatthewFlamm's comment:

Although the filters that it was implemented for specifically used copy=False which would further suggest this as the better default.

Furthermore, I'm only seeing one "filter" using this find_containing_cell, which doesn't modify the point. As for datasets that are using it, it makes sense that we don't copy as we're assigning points, as in its use in core/dataset.py

$ git grep _coerce_pointslike_arg
core/dataset.py:from pyvista.utilities.common import _coerce_pointslike_arg
core/dataset.py:        points = _coerce_pointslike_arg(points)
core/dataset.py:        point = _coerce_pointslike_arg(point, copy=False)
core/dataset.py:        point = _coerce_pointslike_arg(point, copy=False)
utilities/common.py:def _coerce_pointslike_arg(
utilities/geometric_objects.py:from pyvista.utilities.common import _coerce_pointslike_arg
utilities/geometric_objects.py:    points = _coerce_pointslike_arg(points)
  1. One discussion about fixing up StucturedGrid's __init__() and docs, for which I've opened Clean up StructuredGrid documentation and __init__() #2733

That indeed needs to be cleaned up, but outside this PR.

  1. #2697 (comment) which needs input.

Resolved in e552144. Thanks for pointing this out!

tests/test_create_no_copy.py Outdated Show resolved Hide resolved
Copy link
Member

@adeak adeak left a comment

Choose a reason for hiding this comment

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

Still LGTM.

@banesullivan banesullivan merged commit 0d30747 into main Jun 3, 2022
@banesullivan banesullivan deleted the patch/coerce_pointslike_arg_copy branch June 3, 2022 18:15
@adeak adeak mentioned this pull request Jun 4, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Uh-oh! Something isn't working as expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating mesh from Points copies and does not share memory
4 participants