Closed wbalmer closed 7 months ago
Fantastic! Thanks a lot William 🙏!
Did you make any changes to the dTdP_temperature_profile
function from pRT
? If not, then I would suggest to import it from pRT
directly since that package is anyway used.
However, instead of importing at the top of the file, please import it in the function where it is used, because importing pRT
is, just like species
, quite slow. So I only imported function from pRT
when they are actually used.
When the function that contains the import is ran a second (or more) time then it will be fast though (from what I could tell) because it is already part of sys.modules
.
Hi Tomas! I understand. I initially copied the function over intending to make some changes, and also because I was worried that the pRT version 3 release might break the imports, but I think the function is fine as it is right now (and we'll probably have to overhaul the imports anyways when pRT v3 comes out). This latest commit includes what I think is a safe import of the function where it is called in the create_pt_profile
function.
I also made the multinest arguments const_efficiency_mode
, sampling_efficiency
, and evidence_tolerance
, optional arguments in run_multinest
as opposed to hard coded. Is this okay?
I'm working on another commit that will make sure the pt profile is read correctly by read_radtrans.py
A few more suggestions:
update_labels
function of util.plot_util
pt_param
in plot_posterior
(lin 322) so they are skipped with inc_pt_param=False
plot_pt_profile
, but that is probably more work, so fine to leave it out if you don't have time for that!Done! I've updated update_labels
, pt_param
in plot_posterior
, and added the raise where applicable. I added the parameters to plot_pt_profile
in an earlier commit and can confirm it will produce a plot; were there more changes to that function you were thinking of? I am happy to try to implement them :)
I should probably have merged your PR before making changes to the structure of AtmosphericRetrieval
... I can resolve this merge conflict.
I have merged the main
branch into the PR.
@wbalmer, would you mind checking the Files changed once more to see if I didn't accidentally introduce a error somewhere?
I commented out the multinest_kwargs
for now, because for the new structure we can pass the parameters perhaps directly. Something that we can decide later. Sorry for the extra work you did...
Excellent, thanks a lot for the implementation! I will merge the PR 👍
Hi Tomas! This is my attempt to add the dlnP/dlnT and associated default number of nodes/priors on those nodes as detailed in ZJ Zhang's paper. I tested it briefly and it ran okay, but there's still room for some improvement (for instance, ZJ mentions that 6 layers is an arbitrary definition, and more or less could be retrieved).
Let me know what you think - I'm not sure if I missed anything that would make this break e.g. the plots...