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

Remove code duplication in initialization of temperature and composition #1154

Merged
merged 5 commits into from Aug 12, 2016

Conversation

class4kayaker
Copy link
Contributor

@class4kayaker class4kayaker commented Aug 3, 2016

Addresses issue #1145.

The unification on the standard of the n_compos+1 loop is due to wanting to reduce logic duplication.

This pull request also adds a test for the composition normalization due to no existing test using said feature.

@class4kayaker class4kayaker changed the title Remove duplication of constraint computation in initialization Remove code duplication in initialization of temperature and composition Aug 4, 2016
@bangerth
Copy link
Contributor

bangerth commented Aug 4, 2016

/run-tests

@@ -214,6 +91,55 @@ namespace aspect

std::vector<types::global_dof_index> local_dof_indices (finite_element.dofs_per_cell);

#if DEAL_II_VERSION_GTE(8,5,0)
Copy link
Member

Choose a reason for hiding this comment

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

The tester currently only tests deal.II 8.4.0. Did you test that this changed code works with 8.5.0.pre? If you do not have a development version of deal.II I could also do that for you. Let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I did check that the changed code worked for 8.5.0-pre. I will check again with the most recent version of deal.II though.

@gassmoeller
Copy link
Member

Very nice, thanks for the cleaning! I have just a few style issues, and the question if this runs with deal.II 8.5.0.pre. Let me know if you need help with that. Other than that this looks good.

@class4kayaker
Copy link
Contributor Author

Tested with 8.5.0.pre, and there are some variations in behavior, but I believe that these are due to the version change having other effects. I have confirmed that sol_cx_2 and sol_cx_4 tests are passing with the update. Is there a good MMS composition test I can use?

@gassmoeller
Copy link
Member

Not sure what you mean by MMS? Apart from that question this is ready to merge.

@class4kayaker
Copy link
Contributor Author

I was asking if there was a good initial condition test for composition since sol_cx does not explicitly test the values when I checked.

@gassmoeller
Copy link
Member

The composition_active test should be good. Does that work with the recent deal.II?

@class4kayaker
Copy link
Contributor Author

The composition_active test passes with deal.II commit dealii/dealii@7b35fdd, which was the master as August 9, 2016. So that should be good. I wanted a good double check because the passive_comp test did not pass due to things that were reasonable local variations.

@gassmoeller
Copy link
Member

Ok, then lets call it a day. It seems reasonable and passes all tests we tested. Thanks again!

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

Successfully merging this pull request may close these issues.

None yet

3 participants