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

Studio Molecules do not have same start positions when launched/reset #287

Closed
KatieWoe opened this issue May 19, 2020 · 22 comments
Closed

Comments

@KatieWoe
Copy link
Contributor

For phetsims/qa#501. Win 10 Firefox.
I noticed that, when comparing molecules in the state or mirror wrappers, the molecules are in the same position each time. However, when launching from studio with a certain molecule positions, the launched position and the position in the sim upon reset do not match. To see this, put the molecules in an easy to recognize position in the second screen and pause the sim (it must be a non-neon gas). When you launch the sim, look at the position of the atoms/molecules. Hit the reset button to see the change.
moleculepositionnotkept

@KatieWoe
Copy link
Contributor Author

Same on first screen.

@jbphet
Copy link
Contributor

jbphet commented May 20, 2020

There is a specific type for the state of the particle information called MultipleParticleModelIO which @zepumph and I worked on together. This problem has something to do with that class and how it is used in the launch-interact-reset scenario. @zepumph is pretty knowledgeable in this, so I'm adding him as an assignee.

@zepumph
Copy link
Member

zepumph commented May 21, 2020

My first thought was to look at MoleculeForceAndMotionDataSetIO and MultipleParticleModelIO to make sure that they were saving all of the data from the core type that would effect the position/rotation of the particles. Nothing looked suspect at first glance (though I don't know the sim at all).

Next I wonder if there is any unseeded randomness that is causing this to be problematic. It was unlikely since mirror-inputs looks correct. There the downstream sim is actually getting seeded, but that isn't happening with State.

From this I looked through usages of phet.joist.random, and I'm becoming a skeptic that this is a problem. For example looking at MultipleParticleModel.injectMoleculeInternal, there are random values that are not stored in the state there. Should they be? I'm not sure why the positions of the particles need to be stored exactly, but if they do, then likely we need to save the random elements of the particle model too, to be able to load those back on the downstream sim.

@samreid can you speak to the importance of having state restore the perfect positions of these elements?

@jbphet can you?

If it were me, I think I would leave it alone.

@jbphet
Copy link
Contributor

jbphet commented May 21, 2020

@zepumph said:

@jbphet can you [speak to the importance of having state restore the perfect positions of these elements]?

It doesn't strike me as super important, but we did go to a lot of effort to support accurate particle positioning, and it works in the basic launch case, so it is odd that it doesn't work in this case. It seems to me that we should endeavor to fully understand why it doesn't work so that we can be sure that there isn't some deeper issue with the way PhET-iO state is restored.

I guess I'll leave the issue assigned to me and will dig into and figure out what's going on.

@samreid
Copy link
Member

samreid commented May 21, 2020

Is it also failing to save/restore their velocity and angular momentum?

@KatieWoe
Copy link
Contributor Author

@samreid it seems it is. I heated up water molecules so that their temperature was in the thousand Kelvin range, and it launched at the default 400 range. That seems like a much larger deal.

@zepumph
Copy link
Member

zepumph commented May 21, 2020

https://www.facebook.com/ThePocketLab/videos/1092633144442595/

Agreed! @jbphet, would you like to pair on this either today or tomorrow to investigate what isn't getting into the state?

@jbphet
Copy link
Contributor

jbphet commented May 29, 2020

@arouinfar and I reviewed this on 5/29/2020 and we decided that it should be fixed for the upcoming early customer dev release.

@zepumph
Copy link
Member

zepumph commented May 29, 2020

Sorry it fell off my radar.

@jbphet
Copy link
Contributor

jbphet commented Jun 1, 2020

I did some investigation of this issue, and the problematic behavior is due to the order in which state is being restored. The velocities, positions, etc, of the particles are being set prior to the setting of the substance type (e.g. argon, oxygen, neon), and there is an elaborate handler for the setting of the substance that initializes the particles information, thus overwriting the previously restored state.

I met with @zepumph and we discussed this, and it sounds like the phet-io state engine at this point in time wouldn't be able to support the sort of dependency ordering that we need here. It does have some support for order dependence, but not quite what we need in this case. @zepumph offered to add some additional capability, but I'd like to try making the state restoration process very explicitly ordered using MultiParticleModelIO first, so that's the plan for now. I'll implement it, get it working, and then have @zepumph review.

jbphet added a commit that referenced this issue Jun 2, 2020
@jbphet
Copy link
Contributor

jbphet commented Jun 2, 2020

I've made significant progress on this with help from @zepumph by taking more explicit control of the serialization for MultipleParticleModel. As of the previous commit, the original problem described in this issue, where the particles for non-neon substances aren't correctly positioned on a launch from studio, now appears to be resolved. However, we're not quite out of the woods yet. In my testing, I noticed that for oxygen and water the particles evolve quite differently after a launch-unpause-reset-unpause test, and it is very noticeable for oxygen. Through setting breakpoints and logging output to the console, I could see that the rotational state information is not being restored to the same values on reset. The next time I work on this I'll see if I can figure out why.

@KatieWoe
Copy link
Contributor Author

KatieWoe commented Aug 7, 2020

This does look like it might be fixed, but #311 (comment) makes it a bit hard to tell for sure. Leaving open until a fix for that is made.

@jbphet
Copy link
Contributor

jbphet commented Aug 7, 2020

#311 is fixed (I think), I'll leave this open and will put it on the list to verify during the next RC test.

@liammulh
Copy link
Member

liammulh commented Aug 10, 2020

While testing phetsims/qa#525, I noticed that in the mirror inputs wrapper, the top sim's particles don't match the bottom sim's particles.

@jbphet, do you think this warrants a separate issue? If so, please assign me and I'll make one.

@jbphet
Copy link
Contributor

jbphet commented Aug 11, 2020

@muedli - two things: First, yes, what you're describing should be logged as a separate issue. The mirror wrapper works by replaying input events, and doesn't convey other state information between the two instances of the sim, so the mechanisms are fundamentally different from what we've been working to address this this issue. Second, I just tried the mirror wrapper for a few minutes and I didn't see discrepancies in particle positions, so you'll need to be clear in the description about the conditions under which you're seeing divergence.

@KatieWoe
Copy link
Contributor Author

looks ok in rc.2. Will reopen if something comes up.

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

No branches or pull requests

6 participants