Skip to content

Extract profile spherical velocity and data load#386

Open
vivi235711 wants to merge 19 commits intogamer-project:mainfrom
vivi235711:extract_profile_sph_vel_and_data_load
Open

Extract profile spherical velocity and data load#386
vivi235711 wants to merge 19 commits intogamer-project:mainfrom
vivi235711:extract_profile_sph_vel_and_data_load

Conversation

@vivi235711
Copy link
Copy Markdown
Contributor

Enhance GAMER_ExtractProfile with reading hybrid scheme output, improved accuracy, shell-average velocity, and spherical velocity output.

@hyschive hyschive added enhancement tool Useful analysis and visualization tools labels Dec 20, 2024
Copy link
Copy Markdown
Contributor

@hsinhaoHHuang hsinhaoHHuang left a comment

Choose a reason for hiding this comment

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

@vivi235711
Thank you for providing this useful tool.
I have finished the PR review.
Please see my comments when you have time and let me know if they are not clear.
Thanks!

Comment thread tool/analysis/gamer_extract_profile/GAMER_Functions/LoadData.cpp Outdated
Comment thread tool/analysis/gamer_extract_profile/GAMER_Functions/LoadData_HDF5.cpp Outdated
Comment thread tool/analysis/gamer_extract_profile/GAMER_Functions/LoadData_HDF5.cpp Outdated
Comment thread tool/analysis/gamer_extract_profile/GAMER_Functions/LoadData_HDF5.cpp Outdated
Comment thread tool/analysis/gamer_extract_profile/Main.cpp Outdated
Comment thread tool/analysis/gamer_extract_profile/Main.cpp
Comment thread tool/analysis/gamer_extract_profile/Main.cpp Outdated
Comment thread tool/analysis/gamer_extract_profile/Main.cpp
Comment on lines +1708 to +1716
if ( OutputSphere )
{
sprintf( FileName[Var++], "%s", "AveVr" );
sprintf( FileName[Var++], "%s", "AveVtheta");
sprintf( FileName[Var++], "%s", "AveVphi" );
sprintf( FileName[Var++], "%s", "AveWr" );
sprintf( FileName[Var++], "%s", "AveWtheta");
sprintf( FileName[Var++], "%s", "AveWphi" );
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems OutputSphere only supports ELBDM currently. Would we plan to also support HYDRO? The coordinate transformation should also work for the hydro velocity.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Now can support hydro.
May I ask what hydro test problem you use to test this code?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can use Hydro/Plummer test problem.

@vivi235711
Copy link
Copy Markdown
Contributor Author

@hsinhaoHHuang Thanks for your detailed reviewed! I've updated the code based on your comments. Please take a look when you have time.

@hyschive
Copy link
Copy Markdown
Contributor

@vivi235711 Please update this PR to synchronize it with the latest main.

@hsinhaoHHuang Please take another look at this PR (if needed) and approve it if you think it's ready to be merged.

Copy link
Copy Markdown
Contributor

@hsinhaoHHuang hsinhaoHHuang left a comment

Choose a reason for hiding this comment

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

Apologies for my late response. I have reviewed it again and left some comments. Please let me know your thoughts about them when you have time. Thank you!

char Key[MaxString];
sprintf( Key, "FieldLabel%02d", v );

# if ( MODEL == ELBDM)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
# if ( MODEL == ELBDM)
# if ( MODEL == ELBDM )

// Transfer dens and phase to real and imag
# if ( MODEL == ELBDM )
if ( ELBDM_Scheme == 2 )
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
{
{

if ( ELBDM_RichExtrap )
{
// Richardson extrapolation 2 order
GradD[0] = ( 4*_2dh*( Field[p][DENS][k ][j ][ip ] - Field[p][DENS][k ][j ][im ] )
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
GradD[0] = ( 4*_2dh*( Field[p][DENS][k ][j ][ip ] - Field[p][DENS][k ][j ][im ] )
GradD[0] = ( 4*_2dh*( Field[p][DENS][k ][j ][ip ] - Field[p][DENS][k ][j ][im ] )

if ( average_dens != NULL ) delete [] average_dens;
cout << " Bulk CoM velocity = ("<< CMV[0]<< ", "<< CMV[1] << ", "<<CMV[2]<< ")" << endl;

cout << "Remove the motion of center-of-mass by phase shift" << endl;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this message cout << "Remove the motion of center-of-mass by phase shift" << endl; can be put inside # elif ( MODEL == ELBDM ).

As for MODEL == HYDRO, we should be able to do something like this to remove the center-of-mass velocity:

Dens = amr.patch[lv][PID]->fluid[DENS][k][j][i];
VelX = amr.patch[lv][PID]->fluid[MOMX][k][j][i] / Dens;
VelY = amr.patch[lv][PID]->fluid[MOMY][k][j][i] / Dens;
VelZ = amr.patch[lv][PID]->fluid[MOMZ][k][j][i] / Dens;

amr.patch[lv][PID]->fluid[MOMX][k][j][i] = (VelX - CMV[0]) * Dens; 
amr.patch[lv][PID]->fluid[MOMX][k][j][i] = (VelY - CMV[1]) * Dens; 
amr.patch[lv][PID]->fluid[MOMX][k][j][i] = (VelZ - CMV[2]) * Dens; 

# else
# error : ERROR : unsupported MODEL !!
# endif // MODEL
} // FUNCTION : Remove_CMVel
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
} // FUNCTION : Remove_CMVel
} // FUNCTION : Remove_CMVel

double *ELBDM_RhoUr2 = NULL; // Rho*(vr^2 + wr^2) in the virial surface terms
double *ELBDM_dRho_dr = NULL; // dRho/dr in the virial surface terms
double *ELBDM_LapRho = NULL; // Lap(Rho) in the virial surface terms
bool ELBDM_RichExtrap = true; // Richardson extrapolation for the velocity
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel this name may be confusing. I think we didn't really do Richardson extrapolation. What this option means in this code is actually to use a 5-point stencil central difference method to compute the gradient and Laplacian. Although the adopted formula can be derived by Richardson extrapolation, it seems to me that "Richardson extrapolation" is a more general term referring to estimating the value using two different resolutions (not necessarily dh and 2*dh).
Do I understand it correctly?

I would think a name like ELBDM_5PointDerivatives (just an example) would at least be better to convey that the derivatives will be computed by a 5-point stencil instead of a 3-point stencil.

bool GetAvePot = false; // calculate the spherically symmetric gravitational potential
double NewtonG = WRONG; // gravitational constant (must be set if it cannot be determined from the input data)
bool RemoveCMV = false; // remove the center-of-mass velocity
bool OutputSphVel = false; // output the sphere coordinate (r, theta, phi) velocity
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool OutputSphVel = false; // output the sphere coordinate (r, theta, phi) velocity
bool OutputSphVel = false; // output the spherical coordinate (r, theta, phi) velocity


# if ( MODEL == HYDRO )
// evaluate the values on the shell
# warning : WAIT HYDRO !!!
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the calculation is straightforward in HYDRO:

Dens    = Field[p][DENS][k][j][i];
MomX    = Field[p][MOMX][k][j][i];
MomY    = Field[p][MOMY][k][j][i];
MomZ    = Field[p][MOMZ][k][j][i];

CMV[0]  += (double)(dv*MomX);
CMV[1]  += (double)(dv*MomY);
CMV[2]  += (double)(dv*MomZ);
massAll += (double)(dv*Dens);

Comment on lines +718 to +723
GradD[0] = ( 4*_2dh*( Field[p][DENS][k ][j ][ip ] - Field[p][DENS][k ][j ][im ] )
- 0.5*_2dh*( Field[p][DENS][k ][j ][ipp] - Field[p][DENS][k ][j ][imm] ))/3;
GradD[1] = ( 4*_2dh*( Field[p][DENS][k ][jp ][i ] - Field[p][DENS][k ][jm ][i ] )
- 0.5*_2dh*( Field[p][DENS][k ][jpp][i ] - Field[p][DENS][k ][jmm][i ] ))/3;
GradD[2] = ( 4*_2dh*( Field[p][DENS][kp ][j ][i ] - Field[p][DENS][km ][j ][i ] )
- 0.5*_2dh*( Field[p][DENS][kpp][j ][i ] - Field[p][DENS][kmm][j ][i ] ))/3;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you think it would be cleaner and easier to maintain if we wrap the calculation as a function?

void grad( real* field_out, const real* field_in, const int i, const int j, const int k, const double dh)
{
   if ( ELBDM_RichExtrap )
   {
      field_out[0] = (   4*_2dh*( field_in[k  ][j  ][i+1] - field_in[k  ][j  ][i-1] )
                      -0.5*_2dh*( field_in[k  ][j  ][i+2] - field_in[k  ][j  ][i-2] ))/3;
      field_out[1] = (   4*_2dh*( field_in[k  ][j+1][i  ] - field_in[k  ][j-1][i  ] )
                      -0.5*_2dh*( field_in[k  ][j+2][i  ] - field_in[k  ][j-2][i  ] ))/3;
      field_out[2] = (   4*_2dh*( field_in[k+1][j  ][i  ] - field_in[k-1][j  ][i  ] )
                      -0.5*_2dh*( field_in[k+2][j  ][i  ] - field_in[k-2][j  ][i  ] ))/3;
   }
   else
   {
      field_out[0] = _2dh*( field_in[k  ][j  ][i+1] - field_out[k  ][j  ][i-1] );
      field_out[1] = _2dh*( field_in[k  ][j+1][i  ] - field_out[k  ][j-1][i  ] );
      field_out[2] = _2dh*( field_in[k+1][j  ][i  ] - field_out[k-1][j  ][i  ] );
   }
}

real laplacian( const real* field_in, const int i, const int j, const int k, const double dh)
{
   // ...
   return laplacian;
}

Then, we can invoke it simply by

grad( GradD, Field[p][DENS], i, j, k, dh);
grad( GradR, Field[p][REAL], i, j, k, dh);
grad( GradI, Field[p][IMAG], i, j, k, dh);
LapR = laplacian( Field[p][REAL], i, j, k, dh);
LapI = laplacian( Field[p][IMAG], i, j, k, dh);

Comment on lines +1708 to +1716
if ( OutputSphere )
{
sprintf( FileName[Var++], "%s", "AveVr" );
sprintf( FileName[Var++], "%s", "AveVtheta");
sprintf( FileName[Var++], "%s", "AveVphi" );
sprintf( FileName[Var++], "%s", "AveWr" );
sprintf( FileName[Var++], "%s", "AveWtheta");
sprintf( FileName[Var++], "%s", "AveWphi" );
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can use Hydro/Plummer test problem.

@hyschive
Copy link
Copy Markdown
Contributor

@vivi235711

  • The compilation fails for MODEL=HYDRO
  • Please synchronize with the latest main branch.

@koarakawaii
Copy link
Copy Markdown
Contributor

When running the code with MODEL = ELBDM with -V option on, the RMS for AveDens_* is different when -O option is on/off (probably related to this line, which should not touch the density data when -O option is off, but seems to still touch it because NOut seems to be 12 in this case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement tool Useful analysis and visualization tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants