-
Notifications
You must be signed in to change notification settings - Fork 21
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
Changes to plot multimodal posteriors when the option is enabled. To … #451
Changes to plot multimodal posteriors when the option is enabled. To … #451
Conversation
…enable the multi_mode option turn multi_mode=True when loading the runs
…ing-for-individual-mode
…ttps://github.com/xpsi-group/xpsi into 415-posterior-post-processing-for-individual-mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good and works for me. I suggest to keep the default xpsi loaded file so the user can still access it without reloading everything
xpsi/PostProcessing/_runs.py
Outdated
run_IDs.append(run_IDs[vec]+f"{mode_label} {mode+1}") | ||
base_dirs.append(base_dirs[vec]) | ||
use_nestcheck.append(use_nestcheck[vec]) | ||
# Forget about the defaul xpsi loaded file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"defaul" should be "default" here. But I agree @lmauviard that keeping the default X-PSI file could be handy if want to switch to showing the normal corner plots and not wanting to re-load the runs. But if having that is complicated or breaks things, I am fine for leaving it out.
Otherwise it looks good to me and seems to pretty much work for in my tests... I just have 1 out of my 4 runs that sometimes complains that One easy thing that could be added is a docstring to the |
Well, I actually I think there is a bug in this code. If I first load a run with 3 modes for one model and then load another run, with 4 modes, for another model, I get the above-mentioned KeyError when trying to only plot the run (and the model) that has 3 modes. I suspect it for some reason thinks that even that run should have 4 modes. But I am not sure yet what in the code is making that to happen. Here is the full error message:
|
xpsi/PostProcessing/_runs.py
Outdated
base_dirs = base_dirs[1:] | ||
use_nestcheck = use_nestcheck[1:] | ||
|
||
cls.mode_len =len(modes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it it be this line that is the problem? Is this saving the number of modes in some model specific variable, or to something that gets overwritten every time a new model is loaded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the mode_len
should be run-specific, since even different runs can have different number of modes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think there are a few issues. First of all, the number of modes (and the other mode info too) is saved to a class variable that gets overwritten every time runs to new models are loaded. Secondly, this does not seem to work (at least in a general case) either if loading many runs for the same model, since it assumes that all the runs have the same number of modes.
I am not sure if there is an easy way to fix it, but maybe we can just raise errors/warnings if somebody tries to use this with multiple models/runs, or at least warn about this in the docstring. EDIT: I have done that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A solution for the 1st issue would be to make multi-modes variables (like mode_len
) not class variables. This can be achieved by replacing return cls(runs, likelihood, ID, **kwargs)
by something like
Runs = cls(runs, likelihood, ID, **kwargs)
Runs.mode_len =len(modes)
return Runs
For the second issue, this can be fixed by storing the mode_len
and other such values in form of a dictionary, with the keys being the run ID and the values the mode_len
?
If you agree I can do it quickly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, those sounds like good ideas to me. Please go ahead if you like. I think you should then also change mode_label
to not be a class variable either (so that different models can have different labels).
And I guess we still need to assume that if we set multi_mode=True
, all the loaded runs should have been run using the MultiNest set to mmodal=True
? EDIT: Or if not, we can make that an array of booleans instead of one boolean.
Instead of fixing any of the issues I mentioned above, I just updated the branch by adding errors or warnings for situations where the code does not seem to work. You can remove them if you manage (or want) to solve the issues. If not, I am ok for merging this. |
This should fix the issues you mentioned. Now, the parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me and everything I tested seems to work now!
…enable the multi_mode option turn multi_mode=True when loading the runs