-
Notifications
You must be signed in to change notification settings - Fork 274
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
BUG: Track blockless particles #3683
BUG: Track blockless particles #3683
Conversation
While I was implementing the other part of it, I realized that there may be issues with sub-chunking. I think what we need to ensure is that we don't need to read backwards as well as extra-forwards. I think we do not, but I'm keeping this note here just in case. |
I have convinced myself that @jzuhone I think this is ready for review, and I'm happy to walk through it with you if that would help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple comments. I have not reviewed the rest in detail yet. In any case my review shouldn't be required to get this in.
@matthewturk can we set up a time for you to walk me through this? |
@jzuhone Yeah, absolutely. What if we did something on Friday afternoon? |
This looks good to me, but we just need a test, as you say |
What should we do to get this over the finish line? |
@jzuhone I think I need to set aside time to add the test. I will put it on my schedule for today or tomorrow. If you want to try, have at it, but I suspect you've got stuff on your plate too. |
adding this to the 4.0.3 milestone since it looks close to completion. |
The new test data can be found at: http://use.yt/upload/d6e42681 |
I think we just need to get the file on the machine, right? |
New datasets need to be registered in |
Here's the required patch for this branch --- a/yt/sample_data_registry.json
+++ b/yt/sample_data_registry.json
@@ -271,6 +271,12 @@
"load_name": "plt00015",
"url": "https://yt-project.org/data/Laser.tar.gz"
},
+ "LocBub_dust.tar.gz": {
+ "hash": "1ad9728ca982616686d9bd6c1c959fece3c067ecbbba64ff8c2c5bfcc1deed01",
+ "load_kwargs": {},
+ "load_name": "LocBub_dust_hdf5_plt_cnt_0220",
+ "url": "https://yt-project.org/data/LocBub_dust.tar.gz"
+ },
"MagneticumCluster.tar.gz": {
"hash": "403c2224c205a44ee92a5925053be8aef6e5d530d9297c18151cdb0f588c32a7",
"load_kwargs": { |
And here's the companion PR on the website repo : yt-project/website#113 |
I took the liberty to push the patch above to register the new data sample, and confirm that Provided tests pass, I think this is now ready to merge since it was previously approved by @jzuhone |
Ah, looks like the dataset wasn't properly installed on the testing server yet. Maybe @Xarthisius can handle this ? |
@Xarthisius I don't think I know how to do that. :( |
@yt-fido test this please. |
thanks ! |
@meeseeksdev backport to yt-4.0.x |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
manually backported as #3894 |
Backport PR #3683: BUG: Track blockless particles
This is a first step at tracking blockless particles, as noted in #3681. It needs two things before it can go in:
_read_particle_coords
(which I'm hoping someone will have suggestions to improve) needs to be mirrored in_read_particle_fields
.