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

fixing alfven and inchworm #58

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

janirisrodriguez
Copy link
Collaborator

:)

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.

Nice work. 👍

@Yurlungur
Copy link
Collaborator

@brryan if you're happy with this, let's merge in.

@jdolence
Copy link
Member

jdolence commented Dec 8, 2021

actually, I don't think this is quite working for snake yet. Janiris and I threw something in there, but I think something is off with the snake transformation of the b field. was hoping one of you guys could spot what we did wrong.

@Yurlungur
Copy link
Collaborator

Ah ok. I'll look more carefully. At a first glance I didn't see anything wrong.

Copy link
Collaborator

@brryan brryan left a comment

Choose a reason for hiding this comment

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

One question about b^0 otherwise LGTM

for(int d = 0; d < 3; d++){
Bdotu += v(ib_lo+d, k, j, i) * ucon[d+1];
}
Real bcon[NDFULL] = {Bdotu, 0.0,0.0,0.0};
Copy link
Collaborator

Choose a reason for hiding this comment

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

b^0 = B.u/alpha? Also B.u = Gamma B^i v_i but this looks like B.u = B^i u^i?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch. I think this is likely the last thing needed to get this working.

Copy link
Member

Choose a reason for hiding this comment

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

aren't those the same in this case? alpha = 1 and u^i = Gamma v_i since this is Minkowski.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this going to eventually be used for snake/inchworm? The alphas are being used when calculating the b^i just below

Copy link
Member

Choose a reason for hiding this comment

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

this is just the pre-transformation stuff though, isn't it? like no matter what this will always be Minkowski.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right sorry yeah I see this is pre-transformation. OK this should be fine... hm...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops right. So this isn't the bug. I need to sit down with this and go through it with a finer comb.

@jdolence
Copy link
Member

I think this works now, but we should do a convergence study of snake alfven and something in inchworm to confirm before merging.

@brryan
Copy link
Collaborator

brryan commented Feb 14, 2022

I would like to see convergence of just the entropy mode in snake and inchworm as well

@Yurlungur
Copy link
Collaborator

Big effort to dig into this, glad to see it resolved.

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.

4 participants