[frontend] Adaptahop support #2385
Conversation
The test dataset extracts to output_00080_halos (lowercase, no subfolder).
This was causing any `.halo` method to be overriden in `._setup_classes` by a `.halo` method that would return a `GadgetFOFHaloContainer` object.
|
The test failure seems a bit odd -- it says |
This makes the check work even if one of the two is a weakreference.
|
The error was mine, I did not define |
In Python < 3.6, dicts are not ordered so that iterating over key, values is not predictible. This caused the position to be filled with garbage (depending on the fields being read from disk) and then rejected by the selector, so that the shape of the data was depending on what was being read from disk (which influenced the order of the dictionary, which changed which garbage ends up in x, y, z).
|
The appveryor error was fixed in #2395. So I think this is ready for review and acceptance. |
| @@ -1954,7 +1954,7 @@ def __init__(self, center, ds, field_parameters = None, data_source = None): | |||
| self.coords = None | |||
| self._grids = None | |||
|
|
|||
| def cut_region(self, field_cuts, field_parameters=None): | |||
| def cut_region(self, field_cuts, field_parameters=None, locals={}): | |||
There was a problem hiding this comment.
Interesting addition -- good idea.
yt/frontends/adaptahop/io.py
Outdated
| if pcount == 0: | ||
| continue | ||
| pos = self._get_particle_positions() | ||
| yield ptype, (pos[:, i] for i in range(3)) |
There was a problem hiding this comment.
Do we want this generator or should it be a list?
Codecov Report
@@ Coverage Diff @@
## master #2385 +/- ##
===========================================
+ Coverage 49.16% 70.83% +21.66%
===========================================
Files 258 262 +4
Lines 46785 47176 +391
Branches 8196 8264 +68
===========================================
+ Hits 23002 33417 +10415
+ Misses 22330 11714 -10616
- Partials 1453 2045 +592
Continue to review full report at Codecov.
|
| #----------------------------------------------------------------------------- | ||
| # Copyright (c) 2013, yt Development Team. | ||
| # | ||
| # Distributed under the terms of the Modified BSD License. | ||
| # | ||
| # The full license is in the file COPYING.txt, distributed with this software. | ||
| #----------------------------------------------------------------------------- |
There was a problem hiding this comment.
I think we're removing the copyright comment blocks now, right?
There was a problem hiding this comment.
nevermind I realized that your PRs removing them were for yt-4.0.
There was a problem hiding this comment.
I think (2) seems like the best option. There are other frontends that have been added to master that aren't on the 4.0 branch, so they'll need to be cleaned up too. I think it makes sense to do a cleanup of all of it once #1838 is in.
There was a problem hiding this comment.
Fair enough! For reference, I made a mistake, the merge PR is #2172 (so that it appears in its log).
There was a problem hiding this comment.
Ha, clearly I haven't had my ☕️ yet!
|
Hey @brittonsmith do you want to take a look over this PR? |
|
Hi everyone, especially Corentin! I'm so sorry taking so long to review this PR. December and January have been extremely busy months. I've got a major proposal deadline at the end of this week, but I am committing to reviewing this by the end of next week. My apologies for the delay, but thanks for your patience! |
|
The appveyor issues popping up here seem to be the same numpy issues reported in Slack earlier today by @neutrinoceros, and are unrelated to this PR. |
|
@munkm If you think it's OK to include, with the appveyor failure, I say hit the merge button. |
PR Summary
This adds support for the AdaptaHOP halo finder.
PR Checklist