Skip to content
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

Bug fix while using different params for different instruments for fp, p and p1 #112

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

Jayshil
Copy link
Contributor

@Jayshil Jayshil commented Feb 16, 2024

Hi @nespinoza,

The current version of juliet (master and dev branches, which I believe is the latest stable version) has a bug. While fitting for different parameters for fp, p and p1 for different instruments, the sampling will happen only for the last instruments in the list. For example, while fitting for fp_p1_instrument1 and fp_p1_instrument2, the sampling will happen only for the later parameter and the prior would return as posterior for the former.

This is because of a missing if statement while creating self.fp_iname, self.p_iname and self.p1_iname dictionaries in __init__ function of the model class (see, changes below to see what I mean). Because of this missing if statement, those self.fp_iname[p1][ins] etc. dictionaries were kept updating for each ins in the loop and the final value of self.fp_iname[p1][ins] would always correspond to the last instrument in the loop. This PR fixes this issue by adding an if statement. Let me know if you have any questions on this.

Cheers,
Jayshil

@nespinoza
Copy link
Owner

Thanks for this catch! Merging...

@nespinoza nespinoza merged commit 546f87c into nespinoza:dev Feb 16, 2024
@nespinoza
Copy link
Owner

Hey @Jayshil,

I took a deeper look after merging in dev on this, and I remembered what happened here. The problem with your fix is that this fix makes juliet incompatible with previous versions, in which you could just define a prior on p_p1 for instance to fit a wavelength-independant planet-to-star radius ratio.

Just a heads-up that I'm working on this so the final juliet code might look a bit different on this part than your PR.

N.

@Jayshil
Copy link
Contributor Author

Jayshil commented Feb 17, 2024

Hi @nespinoza,

Thanks for informing me! I did not realise that the extra else statement there was for juliet back-compatibility. The changes you made now are a more clear way of doing this. Thanks for doing this!

Cheers,
Jayshil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants