[BUGFIX] Fix particle selection for corner cases of morton index values #2575

matthewturk · 2020-05-04T23:03:20Z

Thanks to @langmm for debugging this with me and for identifying the precise problem. We discovered there was a corner case for filling subregions of mi1 and mi2, which showed up in #2574. This corrects that by choosing the region correctly.

This is a bug identified by some work I'm doing that will add comprehensive testing to particle
selection.

This fixes #2574.

Thanks to Meagan Lang for debugging this with me. We discovered there was a corner case for filling subregions of mi1 and mi2, which showed up in yt-project#2574. This corrects that by choosing the region correctly.

matthewturk · 2020-05-05T14:39:41Z

I would like to get reviews on this (the tests should pass as I had originally pushed backwards logic for is_refined) and hld off on merging until I demonstrate that it fixes functionality; I have a PR incoming on that. Then once that is shown via failing (new) tests, we had hit the button on this, then hit the button on the other one.

langmm

There are a few minor issues (unused variable needs removed and upper range for the method filling coarse cells would need increased), but I also added a few suggestions which might improve performance.

yt/geometry/particle_oct_container.pyx

Co-authored-by: Meagan Lang <cfh5058@gmail.com>

Xarthisius · 2020-05-07T13:17:45Z

After merging this to 2577 tests still fail for me:

======================================================================
FAIL: This checks that we correctly subselect from a dataset, first by making
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/xarth/.local/lib/python3.6/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/xarth/codes/yt-project/yt/yt/frontends/gadget/tests/test_outputs.py", line 129, in test_particle_subselection
    assert_equal(psc.compare_dobj_selection(sp3) , True)
  File "/home/xarth/.local/lib/python3.6/site-packages/numpy/testing/_private/utils.py", line 428, in assert_equal
    raise AssertionError(msg)
AssertionError: 
Items are not equal:
 ACTUAL: False
 DESIRED: True

matthewturk · 2020-05-07T14:17:28Z

The failing test is sp3 which wraps around the domain along all three axes. I'm adding some more test cases to narrow this down. It does not seem to be related to the smoothing length.

matthewturk · 2020-05-07T14:49:36Z

OK, I've changed my mind, it is related to smoothing length, indirectly. Overriding the smoothing length in the io-handler causes this to work.

I believe what is happening is that the periodic smoothing lengths are not being selected inside identify_file_masks, which is what chooses which files are read. I'm looking into that now, as I need to determine if it's not choosing an unrefined cell or a refined cell.

(This is precisely the kind of bug this test was supposed to ferret out, too!)

matthewturk · 2020-05-07T16:56:13Z

The current commit reworks the smoothing length bitmap indexing at the coarse level for periodic boundary conditions. It will not be ready to go in until I've both addressed this in the refined level and tested this.

At present for my test case, this fixes the problem, but it also throws errors for computing the bitmap indices, as it is now setting a few refined cells that are not on the coarse level. Until at a bare minimum that is addressed this cannot go in.

matthewturk · 2020-05-26T21:21:11Z

OK, now that I've explicitly cast to uword in the vendored ewah code, I am out of ideas for why it's failing to compile on appveyor. 😡

…x_select_mi1mi2

matthewturk · 2020-05-29T17:16:23Z

One thing I would note is that I've snuck in an update to using bionic, as per @Xarthisius 's suggestion, on Travis.

matthewturk · 2020-05-29T19:37:10Z

The travis answer test failure is itty bitty, and likely a result of a minor compiler change. I'd like to propose that review commence, and then I update the answer tests.

matthewturk · 2020-06-03T15:15:57Z

PR for answers is here: yt-project/answer-store#4

munkm

Just a couple of questions and one super minor nit. LGTM!

munkm · 2020-06-03T22:24:27Z

yt/geometry/tests/test_particle_octree.py

-        reg._set_refined_index_data_file(
-            sub_mi1, sub_mi2, i, nsub_mi)
+        nsub_mi, coll = reg._refined_index_data_file(
+            coll, pos, hsml, mask, sub_mi1, sub_mi2, i,


Could you use None on l182 here like you did at 218? Or am I missing something subtle that coll needs to be defined here on l179 but not there?

munkm · 2020-06-03T22:28:11Z

.travis.yml

@@ -1,5 +1,5 @@
 language: python
-dist: xenial
+dist: bionic


I must have missed the reason for switching dists in travis. What was that for?

Ups, this slipped through and caused a regression: older GCC (<6) need explicit C++11 flags in the build system, otherwise they will fail to build: #2892
It's not clear to me why this is not set though, because setup.py sets extra_compile_args=["-std=c++11"])... 🤔 Does it need to be added to further Extensions that include the same header?

munkm · 2020-06-03T22:35:02Z

yt/utilities/lib/ewahboolarray/README

@@ -3,6 +3,6 @@ available at:

 https://github.com/lemire/EWAHBoolArray

-Currently this is at revision 80881379f8a582f45dda1be9edfc84d244846427.
+Currently this is at revision 88b25a3345b82353ccd97a7de6064e6c179a7cc2


Ahhh I see this got updated

Update bitmask_version for #2575

More details about the test failure can be found at the URL: http://use.yt/upload/84bbfca3 answer(s) for failed test at URL: http://use.yt/upload/bc305b8c

matthewturk added 3 commits May 4, 2020 12:24

First sketch of a particle selection tester

c3ae3e0

Add in the smoothing length calculations

0299776

Fix particle selection for sub-regions

073fbf9

Thanks to Meagan Lang for debugging this with me. We discovered there was a corner case for filling subregions of mi1 and mi2, which showed up in yt-project#2574. This corrects that by choosing the region correctly.

matthewturk added bug demeshening Removing the global mesh for particles index: particle labels May 4, 2020

matthewturk added this to the 4.0 milestone May 4, 2020

matthewturk requested review from Xarthisius, langmm and munkm May 4, 2020 23:03

Had the logic for is_refined backwards

f3dd249

Updating tests to use particle selection comparison

7a99703

matthewturk mentioned this pull request May 5, 2020

Test particle selections #2577

Closed

langmm reviewed May 5, 2020

View reviewed changes

yt/geometry/particle_oct_container.pyx Show resolved Hide resolved

yt/geometry/particle_oct_container.pyx Outdated Show resolved Hide resolved

yt/geometry/particle_oct_container.pyx Outdated Show resolved Hide resolved

yt/geometry/particle_oct_container.pyx Outdated Show resolved Hide resolved

matthewturk and others added 4 commits May 5, 2020 12:24

Update yt/geometry/particle_oct_container.pyx

df4cf91

Co-authored-by: Meagan Lang <cfh5058@gmail.com>

Update yt/geometry/particle_oct_container.pyx

878e018

Co-authored-by: Meagan Lang <cfh5058@gmail.com>

Update yt/geometry/particle_oct_container.pyx

cc93f19

Co-authored-by: Meagan Lang <cfh5058@gmail.com>

Updating from comments

a8ef879

munkm approved these changes May 5, 2020

View reviewed changes

matthewturk added 3 commits May 7, 2020 09:51

Refine tests a bit

d22947e

Add tests for wrapping on right

934a4eb

Rework periodic smoothing length calculations

402afa0

matthewturk added 2 commits May 7, 2020 13:49

Rework periodic smoothing length calculations

2e39ef1

We need bounds[i][1] + 1 for inclusive loops

0b68faf

matthewturk added 4 commits May 26, 2020 12:55

Check for None in append()

e8ce92b

Changing to uint32_t for Clang

588b50e

Try to be more careful with uint/int distinctions.

6488e10

Explicitly cast to uword

26a4ed4

matthewturk and others added 7 commits May 27, 2020 10:36

Update EWAH to 88b25a3345b82353ccd97a7de6064e6c179a7cc2

e29599d

This fixes a compilation error

9e1f34e

Make this platform-dependent

bd1fb35

Update order-of-include and C++11 for particle_oct_container.pyx

25304b3

Merge branch 'yt-4.0' into fix_select_mi1mi2

1384a35

Update to Bionic as per Kacper's suggestion

e5847d8

Merge branch 'fix_select_mi1mi2' of github.com:matthewturk/yt into fi…

3baa1bc

…x_select_mi1mi2

jzuhone approved these changes Jun 1, 2020

View reviewed changes

Fix a handful of lint and style issues

c85827e

matthewturk mentioned this pull request Jun 3, 2020

Update for #2575 yt-project/answer-store#4

Merged

Updating to new answer-store rev

59290f0

munkm approved these changes Jun 3, 2020

View reviewed changes

munkm merged commit ad8fc19 into yt-project:yt-4.0 Jun 5, 2020

matthewturk added a commit to matthewturk/yt that referenced this pull request Jun 8, 2020

Update bitmask_version for yt-project#2575

ca536a2

matthewturk mentioned this pull request Jun 8, 2020

Update bitmask_version for #2575 #2621

Merged

munkm added a commit that referenced this pull request Jun 8, 2020

Merge pull request #2621 from matthewturk/update_bitmask_version_mi1mi2

c0e8b9c

Update bitmask_version for #2575

This was referenced Oct 8, 2020

Fresh yt installation not importing #2892

Closed

Fix GCC 5 Builds #2939

Merged

matthewturk mentioned this pull request Apr 13, 2021

Memory and CPU usage with small Gadget data sets #3192

Closed

neutrinoceros mentioned this pull request Oct 28, 2021

Odd difference in sphere selection #2574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX] Fix particle selection for corner cases of morton index values #2575

[BUGFIX] Fix particle selection for corner cases of morton index values #2575

matthewturk commented May 4, 2020

matthewturk commented May 5, 2020

langmm left a comment

Xarthisius commented May 7, 2020

matthewturk commented May 7, 2020

matthewturk commented May 7, 2020

matthewturk commented May 7, 2020

matthewturk commented May 26, 2020

matthewturk commented May 29, 2020

matthewturk commented May 29, 2020

matthewturk commented Jun 3, 2020

munkm left a comment

munkm Jun 3, 2020

munkm Jun 3, 2020

ax3l Oct 8, 2020 •

edited

Loading

munkm Jun 3, 2020

[BUGFIX] Fix particle selection for corner cases of morton index values #2575

[BUGFIX] Fix particle selection for corner cases of morton index values #2575

Conversation

matthewturk commented May 4, 2020

matthewturk commented May 5, 2020

langmm left a comment

Choose a reason for hiding this comment

Xarthisius commented May 7, 2020

matthewturk commented May 7, 2020

matthewturk commented May 7, 2020

matthewturk commented May 7, 2020

matthewturk commented May 26, 2020

matthewturk commented May 29, 2020

matthewturk commented May 29, 2020

matthewturk commented Jun 3, 2020

munkm left a comment

Choose a reason for hiding this comment

munkm Jun 3, 2020

Choose a reason for hiding this comment

munkm Jun 3, 2020

Choose a reason for hiding this comment

ax3l Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

munkm Jun 3, 2020

Choose a reason for hiding this comment

ax3l Oct 8, 2020 •

edited

Loading