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

[ENH] Read Rockstar members #4661

Merged
merged 7 commits into from
Oct 4, 2024
Merged

Conversation

cphyc
Copy link
Member

@cphyc cphyc commented Sep 5, 2023

PR Summary

This allow to read member data from Rockstar catalogues.

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.

@cphyc cphyc added code frontends Things related to specific frontends new feature Something fun and new! domain: astro labels Sep 5, 2023
@neutrinoceros
Copy link
Member

@yt-fido test this please

@cphyc cphyc marked this pull request as ready for review September 6, 2023 14:38
@neutrinoceros
Copy link
Member

@cphyc I think adjustments are needed to pass the new test

Copy link
Member

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

I'm really glad someone finally had the courage to implement this. Can you also add a section in the Rockstar frontend docs in doc/source/examining/loading_data.rst?

yt/frontends/rockstar/data_structures.py Outdated Show resolved Hide resolved
yt/frontends/rockstar/data_structures.py Outdated Show resolved Hide resolved
yt/frontends/rockstar/data_structures.py Outdated Show resolved Hide resolved
yt/frontends/rockstar/data_structures.py Show resolved Hide resolved
brittonsmith
brittonsmith previously approved these changes Oct 5, 2023
@brittonsmith
Copy link
Member

I'm not sure where the conflicts came from, but the rest looked good to me. If they can be cleared up, I think this is ready to be merged.

@neutrinoceros
Copy link
Member

@cphyc conflicts probably came from #4275
Happy to merge this once they're resolved

@neutrinoceros
Copy link
Member

@yt-fido test this please

neutrinoceros
neutrinoceros previously approved these changes Oct 6, 2023
@neutrinoceros neutrinoceros enabled auto-merge (squash) October 6, 2023 09:37
@neutrinoceros
Copy link
Member

@yt-fido test this please

@neutrinoceros
Copy link
Member

We have one failing test to deal with

@brittonsmith
Copy link
Member

I cannot reproduce the failing test locally with a Python 3.10 install. Is there a way to get more information?

@cphyc
Copy link
Member Author

cphyc commented Oct 6, 2023

I think this might be real. I need to investigate!

Weird; I have tried locally and cannot reproduce the error on 3.9, 3.10 or 3.11.

Here's the code I tried

import yt

ds = yt.load_sample("rockstar_halos/halos_0.0.bin")

for halo_id, Npart in zip(
    ds.r["halos", "particle_identifier"],
    ds.r["halos", "num_p"],
):
    print(halo_id)
    halo = ds.halo("halos", halo_id)
    assert halo is not None

    # Try accessing properties
    halo.position
    halo.velocity
    halo.mass

    # Make sure we can access the member particles
    assert len(halo.member_ids) == Npart

This should be the code of the failing test, unless I am mistaken.

@cphyc
Copy link
Member Author

cphyc commented Feb 19, 2024

@yt-fido test this please.

@neutrinoceros
Copy link
Member

@cphyc the test server is dead, see discussion on Slack.

@neutrinoceros
Copy link
Member

@yt-fido Test this please

@cphyc
Copy link
Member Author

cphyc commented Mar 4, 2024

I made a bit of progress - it seems that the checksum of the data is not the same on fido as it is on my computer:

# locally
/home/…/rockstar_halos/halos_0.0.bin has sha256sum b'09accfb7ab6f162f29fcfb7c6393e207ba1be9d61cbdc230f17d39f0e90914d7'
/home/…/rockstar_halos/halos_0.1.bin has sha256sum b'42e64f974f7d72f201443d42463fd642f8e37c7c485eac54810e3be8adfd0a22'
# on yt-fido
/mnt/yt/rockstar_halos/halos_0.0.bin has sha256sum b'd4c395708e153ae9ec32873c1b1b13f52ee5e829b6cee34e2ab91c67fe8bdf20'
/mnt/yt/rockstar_halos/halos_0.1.bin has sha256sum b'42e64f974f7d72f201443d42463fd642f8e37c7c485eac54810e3be8adfd0a22'

I should note that the local data was downloaded through yt.load_sample, so it should reflect whatever is on https://yt-project.org/data/.
@Xarthisius do you have an idea what's happening?

@cphyc cphyc added this to the 4.4.0 milestone Apr 12, 2024
@cphyc cphyc changed the title Read Rockstar members [ENH] Read Rockstar members Sep 18, 2024
@cphyc
Copy link
Member Author

cphyc commented Sep 18, 2024

I am quite confident is that the test data isn't the same as the one online. @Xarthisius would it be possible to make sure that the dataset on jenkins is the same as the one obtained when using load_sample("rockstar_halos/halos_0.0.bin")?

I have tested locally and it works like a charm.

@cphyc
Copy link
Member Author

cphyc commented Sep 19, 2024

@yt-fido Test this please

@neutrinoceros neutrinoceros added the enhancement Making something better label Sep 19, 2024
@neutrinoceros
Copy link
Member

adding the "enhancement" label just to make Mergeable Bot happy

@cphyc cphyc added enhancement Making something better and removed enhancement Making something better labels Sep 20, 2024
@cphyc
Copy link
Member Author

cphyc commented Sep 23, 2024

@neutrinoceros @brittonsmith I think the "Mergeable" bot is stuck, unless I am misunderstanding?

@neutrinoceros neutrinoceros added enhancement Making something better and removed enhancement Making something better labels Sep 23, 2024
@neutrinoceros
Copy link
Member

Removing and re-adding a label did the trick.

matthewturk
matthewturk previously approved these changes Sep 24, 2024
self.particle_identifier = particle_identifier

def __repr__(self):
return "%s_%s_%09d" % (self.halo_ds, self.ptype, self.particle_identifier)
Copy link
Member

Choose a reason for hiding this comment

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

Not an f-string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not anymore :)

@matthewturk matthewturk merged commit 1509b29 into yt-project:main Oct 4, 2024
12 checks passed
@cphyc cphyc deleted the rockstar-members branch October 7, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code frontends Things related to specific frontends domain: astro enhancement Making something better new feature Something fun and new!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants