editing defaults for consistency#88
Conversation
michael-petersen
left a comment
michael-petersen
left a comment
There was a problem hiding this comment.
Great idea matching these up. I think we probably want to reduce lmaxfid everywhere (see specific comment), but otherwise great!
There was a problem hiding this comment.
lmaxfid=128 will be really computationally expensive -- I wonder if we want to go the other way and change the defaults elsewhere to a lower value?
There was a problem hiding this comment.
Yeah, lmaxfid=128 is a lot. Do you have a suggestion? 72 or 96 maybe?
There was a problem hiding this comment.
In the past I found diminishing returns after 72 unless really going for it on the other parameters (it takes a lot of integration points to resolve even 96!). Unless we want this to be super-production-ready code, I'd set at 72.
There was a problem hiding this comment.
lmaxfid is set to 128 in src/cylinder.cc and expui/BiorthBasis.H - I'll change it to 72 throughout.
| cxxopts::value<double>(SCSPH)->default_value("1.0")) | ||
| ("RSPHSL", "Maximum halo expansion radius", | ||
| cxxopts::value<double>(RSPHSL)->default_value("47.5")) | ||
| cxxopts::value<double>(RSPHSL)->default_value("1.95")) |
There was a problem hiding this comment.
Another strategy here would be to accept whatever the maximum radius in the input table is. That is probably more user-error proof?
There was a problem hiding this comment.
The main reason for RSPHSL (which could have a better name?) is to guide SLGridSph for tabled models whose density gets close to machine zero, which will break the SL solver. I suppose that choice could be selected from the table itself automatically with some heuristic.
There was a problem hiding this comment.
Right, I guess my hope is that users will have good density models, especially if they are making a set of ICs. But I guess this is a way out if a user had a table they couldn't change (for some weird reason).
There was a problem hiding this comment.
Hm. I'm okay with forcing the user to change their table to something consistent with SL. I'm in favor of reducing the number of free parameters generally. So I'm in favor of your first proposal: get rid of RSPHSL and use the rmax value from the table.
There was a problem hiding this comment.
I don't know how to implement this suggestion but it probably is the best fix
There was a problem hiding this comment.
Are we still using this file? We might flag this up for future jettisoning (or absorption into a pyEXP tool).
There was a problem hiding this comment.
This is a translation test for basis creation. It's a diagnostic for centering.
There was a problem hiding this comment.
Yes, but my thinking is that this won't get exercised as the intended command line tool unless we (a) write instructions, or (b) bring it directly into Python. Or both. Not suggesting we do this now; maybe something to tag in an issue along with the rest of the diagnostic tools. They are good work, and useful!
There was a problem hiding this comment.
Oh. That's what you meant by pyEXP... I'm in favor of keeping this available somewhere. It's an instructive demo. But I don't feel strongly about it.
There was a problem hiding this comment.
Ah - this is not the most relevant file. I percolated nmax = 18 through to the other files, I think (?) that I got all of the relevant files this time
michael-petersen
left a comment
michael-petersen
left a comment
There was a problem hiding this comment.
All welcome changes; I've built some ICs with these updates and everything is a bit more smooth now. Thanks!
3 participants
I edited some of the defaults in gendisk and exp to ensure that they match