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

attempt to fix translation issues in imported models #1029

Merged
merged 5 commits into from
May 30, 2024

Conversation

jhmatthews
Copy link
Collaborator

This PR aims to fix #1025 and #1026

The first change -- for #1025 -- is a bit complex to understand and is made in translate_in_space() and only in imported models. Originally, for imported models, in translate in space we were first calculating the distance to the edge of a box surrounding the wind, and then move it there. Then, when we are inside this box, we do the same again, but now this results in moving the photon to the edge of the box. The crux of this is that photons which "stayed in the wind" were fine, but photons which entered then exit were incorrectly transported to the edge of the box. My proposed fix is to manually increment ds cell by cell in the case that the photon position is inside rhomin and rhomax of the wind.

The second change involves ds_to_plane - the windplane that we use to check is only defined in the northern hemisphere. So rather than doing an additional check, I added something that reflects a test version of the photon ptr for photons that are moving downwards in the southern hemisphere. @kslong you might want to check this seems reasonable, and also I have no idea if this will break the other places we use ds_to_plane.

Probably needs some commenting and could perhaps do with a unit test or two added to test ds routines. I could perhaps add this once Ed's changes are merged.

@jhmatthews
Copy link
Collaborator Author

I'm not sure that these fixes will catch every single problem case, but I have checked that when you "fudge" translate in space by just always translating by 1e7, you get the same results as for this fix. So I think it works at least for the JEDSAD model. here is before and after with this fix. Before:
ntot_onecycle_dev master txt

After:
ntot_onecycle_fix_ds master txt

Note there are still a few streaky looking patterns, but here is the "fudge" case where ds is always 1e7 in translate in space:
ntot_onecycle_fudge master txt

@jhmatthews
Copy link
Collaborator Author

This is basically working now, but there are probably two things to check:
a) does this affect performance much, probably only relevant to imported models
b) this currently fails a test I wrote in which the photon is moved 50% along a path and then has its direction reversed. It means I'm not totally sure how ds_to_plane is supposed to work in this instance, and it still might be working incorrectly for photons that scatter back across a midplane, for example.

@jhmatthews
Copy link
Collaborator Author

James and Knox discussed this at Soton24. They agreed that the old behaviour of ds_to_plane should, in general, be preserved to avoid breaking other bits of code, but there should be a mode variable added, called something like force_positive_z. Thus, in photon2d.c the call would become

ds_to_plane (&plane_m2_far, p, TRUE);

and elsewhere the call would be

ds_to_plane (&plane_m2_far, p, FALSe);

The ds_to_plane function would become

double
ds_to_plane (pl, p, force_positive_z)
     struct plane *pl;
     struct photon *p;
     int force_positive_z;
{
  double denom, diff[3], numer;
  struct photon ptest;

  stuff_phot(p, &ptest);
  if (ptest.x[2] < 0 && ptest.lmn[2] < 0 && (force_positive_z == TRUE))
  {
    ptest.x[2] = -ptest.x[2]; /* force the photon to be in the positive x,z quadrant */
    ptest.lmn[2] = -ptest.lmn[2]; /* force the photon to moving in the positive z direction */
  }

  if ((denom = dot (ptest.lmn, pl->lmn)) == 0)
    return (VERY_BIG);

  vsub (pl->x, ptest.x, diff);

  numer = dot (diff, pl->lmn);

  return (numer / denom);
}

@jhmatthews jhmatthews self-assigned this May 22, 2024
@jhmatthews
Copy link
Collaborator Author

As well as the above, I need to add the translate test to the repo and check it passes.

@jhmatthews
Copy link
Collaborator Author

jhmatthews commented May 30, 2024

I've implemented the above change, which should preserve the old behaviour of ds_to_plane in all cases except when using TRUE in the new mode argument, which is what is used when you want to have a symmetric plane above and below the disk.
This also allows me sing the praises of the unit testing framework which helped me catch a small error in what I'd done initially, thanks @Edward-RSE - I've fixed these tests and they now pass as I expect.

@jhmatthews
Copy link
Collaborator Author

I've checked also that this PR produces smooth photon numbers in the imported JEDSAD model. It produces absolutely identical outputs for the CV standard model as desired. Merging.

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

Successfully merging this pull request may close these issues.

1 participant