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

Add a field to Introspection to query the number of compositional fields. #1505

Merged
merged 1 commit into from May 9, 2017

Conversation

pmbremner
Copy link
Contributor

No description provided.

@bangerth
Copy link
Contributor

bangerth commented May 8, 2017

/run-tests

@bangerth
Copy link
Contributor

bangerth commented May 8, 2017

The tester is having the same problem as always, but I think the patch is ok.

But since I worked on this with @pmbremner I'd prefer if someone else took a look.

@@ -173,6 +173,7 @@ namespace aspect
:
FEVariableCollection<dim>(variable_definition),
n_components (FEVariableCollection<dim>::n_components()),
n_compositional_fields (n_components-dim-2),
Copy link
Member

@tjhei tjhei May 8, 2017

Choose a reason for hiding this comment

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

please use parameters.names_of_compositional_fields.size() instead. I think this can break with melt and/or other additional equations the way you did this.

@pmbremner pmbremner force-pushed the add-field-to-introspection branch from 9f8028d to dfd7975 Compare May 8, 2017 19:10
@pmbremner
Copy link
Contributor Author

Made suggested change. Please have a look

@@ -173,6 +173,7 @@ namespace aspect
:
FEVariableCollection<dim>(variable_definition),
n_components (FEVariableCollection<dim>::n_components()),
n_compositional_fields (parameters.names_of_compositional_fields.size()),
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use parameters.n_compositional_fields as the initializer?

Copy link
Member

Choose a reason for hiding this comment

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

well, that would work too. ;-)

@bangerth
Copy link
Contributor

bangerth commented May 9, 2017

@pmbremner -- would you mind addressing the one comment?

@pmbremner pmbremner force-pushed the add-field-to-introspection branch from dfd7975 to 88256ed Compare May 9, 2017 01:33
@pmbremner
Copy link
Contributor Author

I see a test failed, but it's not clear how this patch affects it. What to do?

@bangerth bangerth merged commit 9c48129 into geodynamics:master May 9, 2017
@pmbremner pmbremner deleted the add-field-to-introspection branch May 9, 2017 15:48
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