fix inadvertently shutting off gas accretion onto sinks

[cmprussell]

fix potentially and inadvertently shutting off accretion of gas particles onto sinks for ntypes=1

Type of PR:
Bug fix

Description:
One-sentence summary: Removed the "iphase" call in an itype variable definition so the potential for the first particle's phase to go negative doesn't inadvertently cease the accretion of all gas particles onto all sinks.

In the subroutine kick of substepping.F90, itype is passed to the subroutine ptmass_accrete in ptmass.F90 as itypei. ptmass_accrete then performs is_accretable(itypei) (line 854) as one of its accretion checks. With the former definition of itype = iphase(igas), the result of itype can either be itype=1 if particle 1 (=igas) is active (which is fine) or itype=-1 if particle 1 is inactive (which is not fine). Passing -1 to itypei in ptmass_accrete then causes all accretion to cease since is_accretable(itypei) = is_accretable(-1) = .false., causing ptmass_accrete to return no accretion.

This only affects ntypes=1 since this originally passed itype-->itypei value is never overwritten if ntypes=1; this means that the originally passed itype definition in substepping.f90/kick is used to check the accretion of every single particle onto every single sink. For ntypes>1, the originally passed itype definition is ignored since each partilces' itype is determined via itype = iamtype(iphase(i)) (line 750, substepping.F90), which correctly assigns a type to the variable itype.

While determining how to modify the incorrect line in kick, I saw several instances of initializing itype via itype = igas in other subroutines in substepping.F90 -- substep_gr (line 161) and get_force (line 902) -- that also had kick's same definition for itype when ntypes>1 further down in their subroutines, so I figured that the subroutine kick could also use itype = igas.

The only reason that can I think of where a more robust solution would be needed is if it's possible to run an ntypes=1 sim where that type isn't gas, e.g. a dust-only sim. In that case, I believe that the line of code in substepping.F90/kick (as well as several other places) should be itype = iamtype(iphase(1)) in order to account for the particles being non-gas particles. But if all ntypes=1 sims are required to be gas-only sims, then itype = igas should work just fine. (If non-gas, ntypes=1 sims are possible, please let me know and then I'll redo this pull request.)

Testing:
This issue came to light while running Galactic Center sims via SETUP=galcen. The plotting of galcenSINK0001N01.ev, which is the point-mass file for the SMBH, showed that the accreted mass (macc, column 12) increased at the start of the simulation and then flatlined, indicating that accretion ceased to function at a certain time. During this flatlining, the regular output files show plenty of material making its way towards the SMBH, with increasingly more of it well within the automatic accretion threshold of r(gas_particle) < f_acc*xyzmh_ptmass(ihacc,1). I eventually tracked this non-accretion behavior down to this single line in substepping.F90; as soon as particle 1 became inactive in the simulation, all accretion ceased.

Upon fixing the mismatch of putting a phase result into a type variable by making line 688 itype = igas, accretion now occurs throughout the Galactic Center simulation as expected.

With this single line modified, I ran the test suite and everything passed.

Did you run the bots? Yes. There were no suggested changes for the one line of code that I changed, so I did not make any of the changes that the bot suggested.

Did you update relevant documentation in the docs directory? no

[danieljprice]

Chris you win the bug fixer of the year award here. I'd noticed a slowdown here but hadn't managed to trace the cause. Thank you so much for finding this!

[cmprussell]

You're welcome!

[Yrisch]

I spotted this issue 1 month ago with IND_TIMESTEPS=yes where accretion was only possible when the first particle was active. I thought that it was fixed with 0e0cc6d. In what case can we have inactive particles without independent timestepping? Sorry... If I knew that I probably would have fixed it :'(

[cmprussell]

Whoops, thanks for pointing this out, Yann. I could have saved myself a good amount of time had I pulled in the latest changes three days more recently than I last did...

I now see that you're storing a phase into a type variable as a way to combine both isactive and type into one, which seems unique in its usage because it does make a difference for the is_accretable call. (All the other instances I found of "itype =" and "iphase" in the same line of code also included an "iamtype" call as well, which aided in my prior confusion.)

Comparing the two implementations, the only difference that I see is that my fix (without your if (ind_timesteps) then; ibin_wakei = ibin_wake(i); itype = iphase(i); endif statement) allows both active and inactive particles to accrete since itype is always positive, while your fix requires the particle to be active to be accreted. Since you both know the code better than I, do either of you think this makes a difference? For example, are there any instances where any necessary bookkeeping steps are only run on accreted particles that are active?

I suppose that my change doesn't need to be undone, but this is certainly up to you all. Line 688's itype is only used (and not overwritten later) when iphase is guaranteed to return a positive result, so iphase(igas) = igas. To me, itype = igas is more intuitive, and it might even futureproof an instance where active/inactive particles can occur without independent timestepping. But obviously if you'd like to undo my change, then please go ahead.

And Daniel, I guess also that my commit doesn't explain the odd accretion behavior you've seen.

[Yrisch]

Yes, it seemed more logical to me that only active particles can accrete when using IND_TIMESTEP. However now that you're pointing this issue, my patch doesn’t make sense if only active particles can enter in the accretion loop where particles be woken if needed... So my patch could be potentially problematic. I'm not sure if it could explain the slow down measured by Daniel but I will reverse my patch just in case. So to answer you, it is certainly needed to allow all (active/inactive) particles to pass accretion tests. Nevertheless, only active ones would be accreted anyway as accretable particles are always in the deepest timestep bins. Your patch is then definitely the good one.
Sorry for the potential performance issue that causes my patch... I'll reverse that as soon as possible.