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

Read simulationarchives version < 3.18 #778

Merged
merged 7 commits into from
Jun 25, 2024
Merged

Read simulationarchives version < 3.18 #778

merged 7 commits into from
Jun 25, 2024

Conversation

hannorein
Copy link
Owner

This allows current versions of REBOUND to read old Simulationarchives (version < 3.18).

@hannorein
Copy link
Owner Author

@dtamayo I did not expect that this small change breaks anything for REBOUNDx (it should only change something when reading in very old simulation archives). Could it be that there is an issue with CI or REBOUNDx?

@dtamayo
Copy link
Collaborator

dtamayo commented Jun 24, 2024

The REBOUNDx CI has some old binaries that it reads in to test for reproducibility, which I'm guessing are the problem. I can update them. Thanks for letting me know

@dtamayo
Copy link
Collaborator

dtamayo commented Jun 24, 2024

Perhaps this is already what this pull request is fixing, but I noticed when running the SavingAndLoadingSimulations.ipynb example in REBOUNDx (after making a new environment and installing the latest REBOUND from pypi, latest REBOUNDx locally from source with pip install -e .) that when I save a REBOUND binary and reload it

sim.save_to_file("reb.bin")
sim2 = rebound.Simulation("reb.bin")

I get these warnings

/Users/dtamayo/opt/miniconda3/envs/rebtest/lib/python3.12/site-packages/rebound/simulation.py:148: RuntimeWarning: Binary file was saved with a different version of REBOUND. Binary format might have changed.
warnings.warn(message, RuntimeWarning)
/Users/dtamayo/opt/miniconda3/envs/rebtest/lib/python3.12/site-packages/rebound/simulation.py:148: RuntimeWarning: You have to reset function pointers after creating a reb_simulation struct with a binary file.
warnings.warn(message, RuntimeWarning)
/Users/dtamayo/opt/miniconda3/envs/rebtest/lib/python3.12/site-packages/rebound/simulation.py:148: RuntimeWarning: Encountered unkown field in file. File might have been saved with a different version of REBOUND.
warnings.warn(message, RuntimeWarning)
/Users/dtamayo/opt/miniconda3/envs/rebtest/lib/python3.12/site-packages/rebound/simulation.py:148: RuntimeWarning: The binary file seems to be corrupted. An attempt has been made to read the uncorrupted parts of it.
warnings.warn(message, RuntimeWarning)

I also checked that it isn't anything REBOUNDx is doing with the function pointers (if I don't add reboundx or any forces I still get the same warnings)

@hannorein hannorein changed the title Reas simulationarchives version < 3.18 Read simulationarchives version < 3.18 Jun 24, 2024
@hannorein
Copy link
Owner Author

hannorein commented Jun 24, 2024

I'm confused.

  1. You're saying that even without this PR and without REBOUNDx something is broken? Can you send me a short piece of code to reproduce that? Just doing
import rebound
sim = rebound.Simulation()
sim.save_to_file("reb.bin")
sim2 = rebound.Simulation("reb.bin")

as you wrote works for me.

  1. Where are the old binary files that the REBOUNDx CI is using?

@hannorein
Copy link
Owner Author

  1. I think I found them. Are they all in reboundx/test/binaries/? I quickly tested a few and they seem to work. Although some are really old (version 3.9). I'll need to look more into which ones fail...

@hannorein
Copy link
Owner Author

Ok. So I think the tests passed by accident before this PR:

  • The SA file was very old. The newer REBOUND version was only able to find one snapshot in the file.
  • You check for machine independence, loading the initial and the final snaphot (sa[-1] and sa[0]), then trying to integrate the initial snapshot to the final position.
  • Because there was only one snapshot, the initial and final times were the same, thus no integration happened, and the checks passed as both simulations were identical (sa[-1] == sa[0]).
  • This PR fixes the initial issue, so now all snapshots are loaded, but as a result the machine independence test fails.
  • I don't know what breaks the machine independence in this case...

@dtamayo
Copy link
Collaborator

dtamayo commented Jun 24, 2024

<Thanks! It looks like the issue causing the warnings are the particle hashes. If I take them out of sim.add, I get no warnings. With hashes, I get the above warnings>

Never mind, need to do some tests to narrow down what's happening. Need to meet wiht a student and will check right after

@hannorein
Copy link
Owner Author

Yeah, I can't reproduce that either.

No rush. I also won't have much time this afternoon...

@dtamayo
Copy link
Collaborator

dtamayo commented Jun 24, 2024

If I run the same commands you sent in a jupyter notebook I get the warnings, but not on the command line...

@hannorein
Copy link
Owner Author

sigh

@hannorein
Copy link
Owner Author

(works for me in a jupyter notebook also)

@dtamayo
Copy link
Collaborator

dtamayo commented Jun 24, 2024

image

This is with python 3.12.4, and just making a new environment and pip install rebound

@hannorein
Copy link
Owner Author

hannorein commented Jun 24, 2024

Done the same with 3.12.3 and it works. 🤷‍♂️ You're using conda, I'm using venv.. can't think of anything else really...

Screenshot 2024-06-24 at 1 21 21 PM

@dtamayo
Copy link
Collaborator

dtamayo commented Jun 24, 2024

I tried getting python 3.12.3 in conda just in case, but got the same warnings:

image

I also tried using virtualenv instead, both installing the latest pypi version, and checking out the sa16 branch and installing that from source locally (both gave identical outputs)

image

I didn't have exactly the same version of python with virtualenv, but can't see how that would matter...I guess we have different versions of Clang (??)

@dtamayo
Copy link
Collaborator

dtamayo commented Jun 24, 2024

Sigh I forgot that every time you save it appends to the binary. So I think the issue was that I'd run this with an old version of rebound, creating reb.bin, and every time I ran it with a new version, it was appending and on load (correctly) said it was saved with a previous version of rebound.

If I remove the binary and then run it, the warnings disappear

@hannorein
Copy link
Owner Author

Ah. Of course! I hadn't thought of that...

So I'll merge this in even though the REBOUNDx machine independence test fails. We can either create a new binary file for the test, or investigate what changed and breaks the test. I think the former is the wiser choice...

@hannorein hannorein merged commit 198527f into main Jun 25, 2024
34 of 56 checks passed
@hannorein hannorein deleted the sa16 branch June 25, 2024 00:21
@dtamayo
Copy link
Collaborator

dtamayo commented Jun 25, 2024

That makes sense. I've updated REBOUNDx to remove wheels for now, and fixed all the simulationarchive unit tests to regenerate each time. Everything works fine now. There's a bunch of different updates bundled in, so I want to spend a few minutes writing some release notes and I'll push a new version to PyPI tomorrow. Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants