aspect icon indicating copy to clipboard operation
aspect copied to clipboard

Published 20 hours ago • verified publisher icon geodynamics

Avoid string comparisons in spherical constant boundary temperature plugin

Open gassmoeller opened this issue 3 years ago • 6 comments

The 'spherical constant' boundary temperature and composition plugins currently does a lot of string comparisons when returning the boundary values, so many that they show up when profiling instantaneous models. The gain for the overall model is not huge, but it is a simple change that makes the plugin more similar to the way the box plugins are written already.

I also removed the minimal_composition and maximal_composition functions from the boundary composition plugin. I think they were copy-pasted from the temperature plugin, because such functions are not defined in the composition interface, and they are nowhere used.

@jdannberg I know you had looked at these plugins before, can you take a look at this?

gassmoeller avatar Jun 25 '21 18:06 gassmoeller

can this wait until after 2.3?

tjhei avatar Jun 26 '21 21:06 tjhei

This does not need to go into 2.3. I improved the documentation and error checks and fixed the tests. There is some awkward behavior in that several places expect this boundary plugin to work for both boundaries even for a full sphere that only has a single boundary. I restored the current behavior, but it looks a bit weird.

gassmoeller avatar Jun 27 '21 15:06 gassmoeller

Ping?

bangerth avatar Jul 21 '21 12:07 bangerth

This will have to wait until I am back from vacation in 2 weeks. Making things consistent with our other plugins will require more or less a rewrite of the whole plugin.

gassmoeller avatar Jul 21 '21 15:07 gassmoeller

Enjoy your vacation, it's well deserved after the hackathon!

bangerth avatar Jul 21 '21 16:07 bangerth

@gassmoeller Want to revive this one as well?

bangerth avatar May 22 '22 01:05 bangerth

@gassmoeller Still interested in this? Or should we close the issue?

bangerth avatar Jul 15 '23 19:07 bangerth

Thanks for reminding me (repeatedly over years :-)). Yes, I would still like to finish this PR. I pushed a new version that should address most comments by staying closer to the original implementation and also preserved legacy behavior (to assign the same value to all compositional fields if only one value is prescribed). This now also adds the functionality to prescribe different values to different compositional fields. From my point this is ready for another review, I just need to wait to update the tester results.

gassmoeller avatar Aug 02 '23 19:08 gassmoeller

Thanks for the review. I accidentally force pushed an older version of this branch, and I dont have access to the new version right now. I will address this next week.

gassmoeller avatar Oct 07 '23 22:10 gassmoeller

@jdannberg Thanks for the review. I added a test.

@bangerth: You requested changes and we cannot merge this PR until you accept the changes. Can you take a look if this is good enough now?

gassmoeller avatar Oct 09 '23 17:10 gassmoeller