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

Added a parameter to switch between fraction of total error and fraction of cells #1085

Merged
merged 2 commits into from Sep 14, 2016

Conversation

lkaratun
Copy link
Contributor

@lkaratun lkaratun commented Jul 1, 2016

Added a parameter to switch between fraction of error and fration of cells for the mesh refinement/coarsening routine

@gassmoeller
Copy link
Member

The implementation looks good, but I would prefer a parameter name like "Refine fraction of cells" or "Refine fraction of cells instead of error". "Use ..." seems to suggest to use a different input parameter than normal. @bangerth, @tjhei what is your opinion?

@@ -519,6 +519,10 @@ namespace aspect
Patterns::Double(0,1),
"The fraction of cells with the smallest error that "
"should be flagged for coarsening.");
prm.declare_entry ("Use fraction of cells", "false",
Patterns::Bool(),
"Use fraction of the total number of cells instead of"
Copy link
Member

Choose a reason for hiding this comment

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

add a space after "of" at the end of the string.

@tjhei
Copy link
Member

tjhei commented Jul 1, 2016

"Refine fraction of cells"

This has the disadvantage that is only mentions "refine" and not "coarsening", maybe "Adapt by fraction of cells"?

@lkaratun
Copy link
Contributor Author

lkaratun commented Jul 2, 2016

I changed the name to "Adapt by fraction of cells"

@tjhei
Copy link
Member

tjhei commented Jul 2, 2016

/run-tests

parameters.refinement_fraction,
parameters.coarsening_fraction);


Copy link
Member

Choose a reason for hiding this comment

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

Please remove one or both (your decision) of these empty lines

@gassmoeller
Copy link
Member

Hi Lev, sorry to come up with more comments after already reviewing it at the hackathon (there was simply too little time there). Can you please change the minor things that I have commented on, and add an entry into /doc/modules/changes.h to make sure you get credit for this change in the next ASPECT release mail. Just copy one of the 'New: ...' entries in there and describe the new feature in a sentence or two. And is there a test that uses the new functionality? If not, please create a very simple one.

@lkaratun
Copy link
Contributor Author

Hi Rene, no worries! I cleaned up the blank lines and added the decription to changes.h. I'm not sure about the tests though - I don't know how to check if any of them use the functionality (I would guess they don't), and I don't really know how to make one. Are there any guidelines for that? Or what would be a good place to start?

@gassmoeller
Copy link
Member

We should write a tutorial about that in the manual at some point, but as a bottom line: Take one of the existing parameter files in the tests/ directory, modify it to use the new feature, check that the new feature does what it is supposed to do, and then just add the parameter file to the tests directory. You will then need to add another folder to that directory that is named like the parameter file, and add the model output files that proof that the feature is working (in your case likely just the file log.txt, and statistics. rename log.txt to screen-output for historical reasons). The test and output files should be as small and quick to run as possible. Let me know if you get stuck at some point.

@gassmoeller
Copy link
Member

Hi Lev,
thanks for updating the test output. You will still need to rebase this branch to a recent master branch of ASPECT. Maybe you have done that with your local master branch, but that one is likely out-of-date (this also explains the PETSC errors, @tjhei just recently fixed that again on the master branch). So the procedure would be:

  1. Switch to your local master branch (git checkout master)
  2. Update your local master branch (e.g. git pull upstream master)
  3. Switch back to your local branch
  4. Rebase your local branch to the newly updated master branch (git rebase master). While at it, you can also squash your commits into a single commit by doing git rebase -i master and then select to squash all but the first commit (change 'pick' into 'squash' for every commit but the first in the list of commits). You will likely get a conflict in this stage that you must fix manually and then do a 'git rebase --continue'
  5. Reupload as before with git push -f origin branch_name.

Changed name to adapt_by_fraction_of_cells

Cleaned up blank lines

Added description to changes.h

Added a test

removed the output directory parameter from the test .prm file

Revert "removed the output directory parameter from the test .prm file"

This reverts commit a27a2f4.

Patched output files
@gassmoeller
Copy link
Member

The one failing test is due to changes on the master branch that happened since you opened this pull request. Just apply the changes.diff file from the tester one more time (or do the changes by hand) and this should be good to go.

@lkaratun
Copy link
Contributor Author

Thanks Rene!

@gassmoeller
Copy link
Member

Ready to merge. Nice! Thanks for contributing 👍

@gassmoeller gassmoeller merged commit 7a1964b into geodynamics:master Sep 14, 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

3 participants