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

plugin for initial temperature condition for ellipsoid chunk geometry #720

Merged
merged 20 commits into from May 12, 2016

Conversation

trajaona
Copy link
Contributor

This is a plugin for initial conditions.


/** This initial condition is designed for the 3D ellipsoid chunk geometry
* model. It discretizes the model domain into two regions separated by an
* isotherm below with the temperature increases adiabatically. The user
Copy link
Contributor

Choose a reason for hiding this comment

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

with -> which

* temperature increases adiabatically while above the adiabatic
* boundary the temperature linearly increases from a surface temperature
* (273.15 K or 0 degree C) to an isotherm (1673.15 K or 1600 degree C)
* that is the adiabatic boundary.
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant to move this documentation to the class declaration, not to the namespace declaration. I think it would also be useful if you merged the documentation, not just collated it.

double initial_temperature (const Point<dim> &position) const;

/**
* declare the parameters that this class needs
Copy link
Contributor

Choose a reason for hiding this comment

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

d -> D

@bangerth
Copy link
Contributor

Some other comments:

  • We usually try to name the class and the section of the parameter file with the same words. Can you adapt this? (We also try to keep class name and file name in sync.)
  • I think the name "adiabatic boundary isotherm" may not be the best. It doesn't actually mean anything to me. Is this a standard technical term? If so, maybe it'd be worth explaining it in the descriptive section at the bottom of the .cc file. If it isn't, maybe we can find a better name?

I (or others) might have other comments once the current ones are addressed.

@trajaona
Copy link
Contributor Author

The adiabatic boundary isotherm is not a standard technical term. I will think about a better name for it.

{
prm.enter_subsection("Adiabatic boundary isotherm");
{
prm.declare_entry("Adiabatic Boundary Filename",
Copy link
Contributor

Choose a reason for hiding this comment

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

We typically capitalize only the first word of an option (just like for section names).

@bangerth
Copy link
Contributor

Let me know when you're done rewriting things and we'll give it another once-over.

Every patch is currently checked whether it satisfies the correct indentation, simply because that makes the entire code base look uniform. We do this using a program called astyle, which you can find here: http://astyle.sourceforge.net/

Can you install version 2.04 (NOT the newer versions) on your machine in a place where it will be found when calling astyle on the command line, and then call ./doc/indent in the aspect directory? This indents all files using the default indentation style, after which you can commit and push the patch once again. This should make the auto-checker happy.

@trajaona
Copy link
Contributor Author

Just to let you know that I pushed the updated patch.

@trajaona
Copy link
Contributor Author

Just a reminder that I have made these changes.

On Jan 26, 2016, at 10:48 PM, Wolfgang Bangerth notifications@github.com wrote:

Let me know when you're done rewriting things and we'll give it another once-over.

Every patch is currently checked whether it satisfies the correct indentation, simply because that makes the entire code base look uniform. We do this using a program called astyle, which you can find here: http://astyle.sourceforge.net/ http://astyle.sourceforge.net/
Can you install version 2.04 (NOT the newer versions) on your machine in a place where it will be found when calling astyle on the command line, and then call ./doc/indent in the aspect directory? This indents all files using the default indentation style, after which you can commit and push the patch once again. This should make the auto-checker happy.


Reply to this email directly or view it on GitHub #720 (comment).

@bangerth
Copy link
Contributor

@gassmoeller -- talking to @trajaona he says that he's made some changes and would appreciate if you could take another look.

@bangerth bangerth self-assigned this Mar 24, 2016
@@ -144,27 +109,21 @@ namespace aspect
double
AdiabaticBoundary<dim>::initial_temperature (const Point<dim> &) const
{
Assert (false, ExcNotImplemented());
AssertThrow (false, ExcMessage ("The 'adiabatic boundary' initial temperature plugin is only implemented for 3d cases."));
Copy link
Member

Choose a reason for hiding this comment

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

This function is a bit confusing, because it is not a template specialization (I guess it is only called in 2d, because you provide a specialization for 3d below). Can you change it to the same format as the AdiabaticBoundary<2>::get_isotherm_depth() function above? I.e. please make this function a template specialization for dim==2.

@gassmoeller
Copy link
Member

Sorry for the delay, I did not see this PR was updated (I only get messages when somebody writes a comment, not when commits are pushed here).
I left some minor comments at the individual commit, I hope you see them although they do not show up in the conversation here. In general I think this PR has improved a lot. Please address the minor issues and take a look at why the test is failing (it seems it can not read the test data file, did you check that in?).
When that is done, please post me a note, and I will take a final look.

@trajaona
Copy link
Contributor Author

@gassmoeller-- My apologize for late reply. Our cluster was down for a while so I could not test the changes I have made on the PR. I have done the edits according to your comments. I have also increased the grid size the lithospheric thickness data to 0.5x0.5 degree since I think this resolution will be enough for the test.

@@ -135,10 +137,13 @@ namespace aspect
{
prm.enter_subsection("Adiabatic boundary");
{
prm.declare_entry ("Data directory", "$ASPECT_SOURCE_DIR/tests/adiabatic_boundary/",
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 prefer if the default value could be set to "$ASPECT_SOURCE_DIR/data/initial-conditions/adiabatic-boundary/", and you would move the test data file there as well. This is how we handle it for all the other tests that need data files, and then all data is in one subfolder (aspect/data).

@gassmoeller
Copy link
Member

No apologies needed 😄. I have two last minor details and a few typos, after that this is good to go.

@gassmoeller
Copy link
Member

Ok, great. The code is good to go. There are two small things left (I had not noticed before, or they just came up with your latest commits): You will need to update your test results to succeed with a current master (I think Timo changed some output format for the number of iterations and updated all already merged tests). And you commited a file 'tests/adiabatic_boundary/.DS_Store' that does not belong there, probably it is something your operating system or file manager created and you accidentally added it, please remove that. After that we are ready to go, thanks for all the work 👍

@gassmoeller
Copy link
Member

Sorry for all the comments, but two more things: There is another .DS_Store in source/initial_conditions/.DS_Store, please remove that as well. And please add an entry to doc/module/changes.h to document your addition. This way we can properly anounce the new feature at the next release. Just copy one of the other entries and write a sentence or two about your plugin. Thanks!

@gassmoeller
Copy link
Member

Ok, looks great. Thanks a lot 👍

@gassmoeller gassmoeller merged commit 3d733a0 into geodynamics:master May 12, 2016
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.

None yet

4 participants