Add elevation output for each Cartesian point#790
Add elevation output for each Cartesian point#790alanjyu wants to merge 3 commits intoGeodynamicWorldBuilder:mainfrom
Conversation
gassmoeller
left a comment
gassmoeller
left a comment
There was a problem hiding this comment.
The change itself looks ok to me (to add elevation output), but I have a few questions before proceeding with this PR, see my comments.
| } | ||
| case 7: // elevation | ||
| { | ||
| if(!std::isnan(output[entry_in_output[i_property]])){ |
There was a problem hiding this comment.
Are you sure this should not be:
| if(!std::isnan(output[entry_in_output[i_property]])){ | |
| if(std::isnan(output[entry_in_output[i_property]])){ |
Do you want to check here if the output is not a number in order to then set it to some number?
There was a problem hiding this comment.
That was a mistake. This is addressed in Commit 0599baa. Thank you!
| Cedric Thieulot | ||
| Cedric Thieulot, | ||
| Yijun Wang, | ||
| and | ||
| Yijun Wang | ||
| Alan Yu |
There was a problem hiding this comment.
I think you created your branch from your other branch (the one used for #783) that is why this change shows up in this PR. Find me to talk about how to fix this.
There was a problem hiding this comment.
I have closed that pull request (used for a tutorial).
| //std::cout << "vel=" << output[entry_in_output[i_property]] << ":" << output[entry_in_output[i_property]+1] << ":" << output[entry_in_output[i_property]+2] << std::endl; | ||
| break; | ||
| } | ||
| case 7: // elevation |
There was a problem hiding this comment.
this may be unrelated to your pull request, but why is elevation 7 and not 6? Why is 6 missing?
There was a problem hiding this comment.
Case 6 is handled by @Djneu and is related to the density of the compositional field.
MFraters
left a comment
MFraters
left a comment
There was a problem hiding this comment.
Given that #778 has been merged, I think I would be best to reopen #783 because that can get merged easily and updating the authors list is not really related to this pull request (Add elevation output for each Cartesian point). I do feel there is some stuff missing here which I thought you worked on. Did you not push that, or do I misremember?
|
I have implemented and tested this function, and it works as intended. However, an issue came up where if you set the feature min depth to something negative, the composition min depth is still set to zero. During the test, a point at negative depth (positive height) will not pick up the composition field unless its min depth matches that of the feature. I am still working on that fix, then will push this along with the test model. |
|
Thanks for the update. Let me know if you want to have a chat about the issue on Monday or Tuesday |
3 participants
Added an output case (case 7) in world.cc, then for the variable calculate the elevation (minimum depth) value at each Cartesian point.