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

last version of the code breaks the wind setup when isink_radiation=1 #538

Closed
lsiess opened this issue May 1, 2024 · 14 comments · Fixed by #539
Closed

last version of the code breaks the wind setup when isink_radiation=1 #538

lsiess opened this issue May 1, 2024 · 14 comments · Fixed by #539
Labels
bug Something isn't working

Comments

@lsiess
Copy link
Contributor

lsiess commented May 1, 2024

After the addition of the 4th order scheme (April 22, hash 282e51), the wind setup does not work anymore when isink_radiation = 1

@danieljprice danieljprice added the bug Something isn't working label May 2, 2024
@Yrisch
Copy link
Contributor

Yrisch commented May 2, 2024

Hi,
For now, you can force the code to work with leapfrog (set use_fourthorder to false). It should resolve the issue temporarily. I checked the routine called for sink radiation in the new substep force routine and it needs an update to use the extrapolation method (mandatory for 4th order). The position of gas and sink particles should be slightly shifted following Chin 2007. I will try to do that in the next few days. Which test I can try to validate the new implementation ?

@lsiess
Copy link
Contributor Author

lsiess commented May 2, 2024

Hi,
An easy test is to run a wind simulation with an old version of Phantom that worked (e.g. 63d8aef) and compare the results with those obtained with the 4th order scheme.
To generate the code : writemake.sh wind > Makefile
and attached are the .in and .setup files
wind_setup.txt
wind_in.txt

Thanks!

@Yrisch
Copy link
Contributor

Yrisch commented May 2, 2024

Thanks ! I'll try to fix that soon !

@Yrisch
Copy link
Contributor

Yrisch commented May 2, 2024

Can you precise a bit more what is wrong now in this setup ? I tried both methods but I can't see a major difference...

@lsiess
Copy link
Contributor Author

lsiess commented May 2, 2024

Hi Yann

If you plot the radial velocity vr = f(r)
Attached is what I obtain : in red old version in black new one

image

@danieljprice danieljprice changed the title last version of the code brakes the wind setup when isink_radiation=1 last version of the code breaks the wind setup when isink_radiation=1 May 2, 2024
@danieljprice
Copy link
Owner

I had a look in the code and could not spot any obvious errors here, except maybe that tau is not updated? Is tau used for this test?

@lsiess
Copy link
Contributor Author

lsiess commented May 2, 2024

No, isink_radiation should only add the radiation pressure force : kappa L/(4 pi r^2 c)

@danieljprice
Copy link
Owner

yes but there is the e^(-tau) term used in some cases

@lsiess
Copy link
Contributor Author

lsiess commented May 2, 2024

tau should only be needed in Mat's ray tracing, i.e. if iray_resolution /= -1.
That we have not checked either!

@Yrisch
Copy link
Contributor

Yrisch commented May 2, 2024

the issue comes from the radiation pressure force that is not accounted during the middle force (using extrapolation) with FSI... I forgot to make a patch for this part of the code. I'm working on it.

@Yrisch
Copy link
Contributor

Yrisch commented May 2, 2024

Ok I made a patch, but I had to merge get_rad_accel_radiation routine with my main gas force loop. was there a specific reason that the two forces were calculated separately in the previous code ?

@Yrisch
Copy link
Contributor

Yrisch commented May 2, 2024

Should be fixed with #539

@danieljprice
Copy link
Owner

see comment on #539, it would be much nicer for this to stay inside the get_force routine if possible. Do we understand why there is a difference between calling it there and separately in the step routine?

@Yrisch
Copy link
Contributor

Yrisch commented May 3, 2024

I probably explained my patch badly... The radiation pressure forces are still computed inside the get_force routine. Before that, it was calculated outside the gas force loop (but still inside get_force). Now these radiation forces are computed inside gas loop of the get_force routine. I didn't understand why it wasn't the case in the previous version of the code (before FSI)...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants