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

Adding Cosmology Example #260

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

Adding Cosmology Example #260

wants to merge 15 commits into from

Conversation

AreefW
Copy link
Contributor

@AreefW AreefW commented Sep 19, 2024

I created a new directory Examples/Cosmo and added all relevant files for doing a cosmology example. The example simulates expanding FLRW spacetime with a sinusoidal scalar field and quadratic potential. We choose a mass of the scalar field as equal to the wave number such that the constraints are satisfied initially. The example also provides how to do a lineout extraction from the simulation and how to use CosmoHamTaggingCriterion.

Note that CosmoAMR.hpp and CosmoMovingPunctureGauge.hpp were added to Source/Cosmology. CosmoHamTaggingCriterion.hpp could be found in Source/TaggingCriteria.

@KAClough KAClough added the enhancement Modification of existing feature/general improvement label Sep 19, 2024
Copy link
Member

@KAClough KAClough left a comment

Choose a reason for hiding this comment

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

Looks great! A few minor comments on comments and naming but all the main bits are fine. Once this is updated it can go to Josu for review!

{
}

//! The compute member which calculates the constraints at each point in the
Copy link
Member

Choose a reason for hiding this comment

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

update comment!

rho_scaled = emtensor.rho / pow(vars.chi, 3. / 2.);
S_scaled = emtensor.S / pow(vars.chi, 3. / 2.);

// Write the constraints into the output FArrayBox
Copy link
Member

Choose a reason for hiding this comment

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

I would say "into the output vector of cells" in the comment

current_cell.store_vars(S_scaled, c_S_scaled);
current_cell.store_vars(K_scaled, c_K_scaled);

// store_vars(out, current_cell);
Copy link
Member

Choose a reason for hiding this comment

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

remove this comment

m_p.scalar_field_mode)),
m_state_new, m_state_new, INCLUDE_GHOST_CELLS);

fillAllGhosts();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just add a comment to say this Gamma Calculator is not needed since the data is conformally flat, but it is left for generality.

scalar_field, m_dx, m_p.G_Newton);
BoxLoops::loop(cosmo_diagnostics, m_state_new, m_state_diagnostics,
EXCLUDE_GHOST_CELLS);
// Assign initial rho_mean here
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make the calculation of initial_rho_mean a public function in the class InitialScalarData? That way it is clear that this is specific to that choice of data and initial params, and people can go there to see how it is calculated.

m_puncture_tracker.set_interpolator(a_interpolator);
}

void set_K_mean(double a_K_mean) { m_K_mean = a_K_mean; }
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment for each function as not completely obvious, or just one comment for all

/// class
/**
* This class implements a slightly more generic version of the moving puncture
* gauge. In particular it uses a Bona-Masso slicing condition of the form
Copy link
Member

Choose a reason for hiding this comment

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

update comment

double m_K_mean;

public:
CosmoMovingPunctureGauge(const params_t &a_params) : m_params(a_params) {}
Copy link
Member

Choose a reason for hiding this comment

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

I think you should pass K_mean in the constructor, even if you default it to zero, as otherwise you risk evolving with it unset.


// Divide Ham_abs_sum by rho_mean
data_t criterion = Ham_abs_sum / m_rho_mean * sqrt_gamma * m_dx;
auto regrid = simd_compare_gt(r, m_rad);
Copy link
Member

Choose a reason for hiding this comment

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

Add some comments on what this is doing with the rad parameter - and change the name!

data_t criterion = Ham_abs_sum / m_rho_mean * sqrt_gamma * m_dx;
auto regrid = simd_compare_gt(r, m_rad);

// data_t criterion = 0.0;
Copy link
Member

Choose a reason for hiding this comment

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

remove this?

@KAClough
Copy link
Member

KAClough commented Oct 14, 2024

A few more misc points:

  • I tested the code compiled ok and ran on cosma8, and the python files work too - except they assume that a data directory cheap_data exists but this isn't created - need to amend to data/

  • This is now 2 commits behind the main you will need to rebase... ask Sam if you want to do it properly, but I don't mind if you just merge!

@AreefW AreefW force-pushed the feature/cosmo_example branch from e71af04 to 801f623 Compare October 22, 2024 21:17
@AreefW
Copy link
Contributor Author

AreefW commented Oct 22, 2024

I've resolved Katy's comments, rebased the commits, and updated the branch. Now, (I think) it's ready (?) for @JCAurre to review. 🥺

@AreefW
Copy link
Contributor Author

AreefW commented Oct 29, 2024

@JCAurre The wiki page of running Cosmo example is also done. See https://github.com/GRTLCollaboration/GRChombo/wiki/Running-the-Cosmo-example

m_time);
rho_extraction.execute_query(&interpolator,
m_p.data_path + "rho_" + "lineout");
// Ham lineout
Copy link
Member

@KAClough KAClough Dec 2, 2024

Choose a reason for hiding this comment

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

Will this run on a restart? Maybe it does, but I think it might be better to put it into the restart function, as I did on the ScalarFieldBH example - take a look at that and see what you think. I don't think we need this at every timestep. Otherwise the changes look good! Maybe you can also fix the x axis on my Ham plot which idk how to do :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Modification of existing feature/general improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants