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

Hyperexponential coordinates #166

Merged
merged 8 commits into from
May 4, 2023
Merged

Hyperexponential coordinates #166

merged 8 commits into from
May 4, 2023

Conversation

brryan
Copy link
Collaborator

@brryan brryan commented Apr 19, 2023

PR Summary

An alternative (going back to ~Tchekhovskoy+ 2011) to having an outer boundary near a disk is to use hyperexponential coordinates such that, beyond some break radius, zones grow radially in size very rapidly with radius.

This PR implements the hyperexponential coordinates based on how they are implemented in KHARMA.

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by calling scripts/bash/format.sh.
  • Explain what you did.
  • Do at least some rudimentary testing.
  • Make sure this resolves to the flatgcov treatment in Maintain sanity (or madness) #165

@brryan brryan requested review from Yurlungur and bprather April 19, 2023 22:03
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably make sure this doesn't break anything and also add a test. Other than that, 👍 nice improvement.

I would like someday to fully compactify the domain and go all the way to physical infinity at $X1=1$ or something. But that can be a problem for future us.

src/geometry/mckinney_gammie_ryan.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@bprather bprather left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code sharing!

src/geometry/mckinney_gammie_ryan.hpp Outdated Show resolved Hide resolved
@@ -191,6 +202,9 @@ class McKinneyGammieRyan {
Real x0_ = 0; // start point of smooth region
Real smooth_ = 0.5;
Real norm_;
Real hexp_br_ = 1000.;
Real hexp_nsq_ = 1.;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I discovered when I tried to run things with this value, nsq=1 is dangerous (it is the default in HARMPI and what was used in Tchekhovskoy '11 afaict though).
The first derivative of the function r(X1) is discontinuous for this value, and it exacerbates the problems with passing material over the break radius. The effect may be much less if you use numerical first derivatives, as HARMPI does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, I just pulled out the default values from kharma (as I interpreted them). Could you remind me what your preferred values are and I can update these defaults?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it was default in KHARMA because I lifted that value straight from HARMPI. I've changed it but not committed the change yet. n=2 is a better default, but still suffers a degree of the same problem in my testing. Personally I think the "best" value is the highest one you can get away with, i.e. which does not spaghetti the outer zones into complete oblivion for your case. I haven't found a combination that eliminates problems moving material over r_br though.

TBH for AGN I'm trying out the atanh trick, and a couple more @Yurlungur gave me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😁

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks -- updated to hexp_nsq = 4

@brryan
Copy link
Collaborator Author

brryan commented Apr 20, 2023

Code sharing!

Does copy and pasting count? Lol

@bprather
Copy link
Collaborator

Code sharing!

Does copy and pasting count? Lol

You changed the names!

(Honestly I like your naming better, and hadn't considered adding HExp to FMKS since I'm using it so differently, so I'll probably steal it back even more literally)

@brryan
Copy link
Collaborator Author

brryan commented Apr 20, 2023

We should probably make sure this doesn't break anything and also add a test. Other than that, 👍 nice improvement.

The original FMKS unit test was actually good for this -- the answer is unchanged in the region below the hyperexponential break radius. I also updated that test to sample a region outside the break radius.

2D torus runs fine for a short time, and grid looks correct.

@brryan brryan changed the title Draft: Hyperexponential coordinates Hyperexponential coordinates Apr 20, 2023
scripts/python/phoedf.py Outdated Show resolved Hide resolved
@Yurlungur
Copy link
Collaborator

Is this ready to go in @brryan ?

@brryan
Copy link
Collaborator Author

brryan commented May 3, 2023

@Yurlungur This is ready to go in, but I think it would be easiest to merge #165 first when everyone thinks thats ready, then I can deal with the merge conflicts and undoing the less-good fix to gcov ghost cell pruning in phoedf.py that is currently in this PR if thats OK with you

src/geometry/fmks.cpp Outdated Show resolved Hide resolved
@brryan brryan merged commit ca02a87 into main May 4, 2023
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.

3 participants