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

conflict between reffect_effect_switch and refl_num in legacy wrapper #187

Open
kecnry opened this issue Dec 15, 2016 · 7 comments
Open
Assignees
Labels

Comments

@kecnry
Copy link
Member

kecnry commented Dec 15, 2016

secondary luminosities need to be updated within phoebe1. Apparently this happens automatically for detached but not contact systems?

For the 2.0 release - we either need to get to the bottom of this or throw an error saying we do not support calling phoebe1 for contact systems (which also makes testing difficult). We also need to understand why this isn't causing issues for the detached case.

@kecnry kecnry added the bug label Dec 15, 2016
@kecnry kecnry added this to the 2.0 milestone Dec 15, 2016
@bpablo
Copy link
Contributor

bpablo commented Jan 10, 2017

This bug report is actually incorrect. What is actually happening is the following:

In newer versions of phoebe2 we have gotten rid of the reffect_effect_switch. Basically the assumption is that if a star has reflections then reflection is on. Most of the time this is fine, except if you have multiple reflections in your file but have the reffect_effect_switch turned off. Then phoebe2 reads it in sees that there are multiple reflections and passes a system to legacy with the reflection switch turned on. This is pretty straightforward, but the fix isn't. There are in my mind two ways this situation can be handled:

1.) output a warning saying something like "reflection switch is off but multiple reflections are listed. Since phoebe2 doesn't distinguish the reflection will be on by default. If you do not wish this to be so then set refl_num to 0."
2.) If reflection effect is off pass the number of reflections to phoebe2 as 0 regardless of what the number actually is.

Which is preferable?

@aprsa
Copy link
Contributor

aprsa commented Jan 10, 2017

I vote the latter. If the reflection effect is off, then the number of reflections is a dummy variable.

That said, reflection in WD is never truly off: the "simple" reflection model is always used, it just doesn't do a detailed computation of multiple reflections. Thus, the "reflection" switch in phoebe 1 should be understood as "detailed reflection" switch. How are you dealing with switch=1, number of reflections=1?

@bpablo
Copy link
Contributor

bpablo commented Jan 10, 2017

The current logic is as follows:

refl_num = 0:
albedo both components is set to zero
reflection effect is set to 0

refl_num = 1:
reflection effect is set to 0

refl_num= multiple:
reflection effect is set to 1

@aprsa
Copy link
Contributor

aprsa commented Jan 10, 2017

When reflection is on, and refl_num=1, phoebe 1 computes reflection using the inverse square law, so we are not consistent with the previous functionality, but at the same time we don't support this "simple" reflection model. I am adding @horvatm to this discussion.

@kecnry kecnry changed the title coupled pblums for contacts not handled by wrapper conflict between reffect_effect_switch and refl_num in legacy wrapper Jan 12, 2017
@bpablo
Copy link
Contributor

bpablo commented Jan 13, 2017

The wrapper has now been changed to the second option: automatically set refl_num to 0 when reflection switch is off. It also raises a warning alerting you to this fact. I am not closing this thread though, as there is still some question as to how to handle refl_num=1.

@kecnry
Copy link
Member Author

kecnry commented Jan 13, 2017

great, then I'll just switch from 2.0 to future since we decided to defer the "simple" reflection case from this release

@kecnry kecnry modified the milestones: future, 2.0 Jan 13, 2017
@kecnry kecnry modified the milestones: future, redistribution support Sep 27, 2017
@horvatm horvatm removed their assignment Jul 13, 2018
@kecnry
Copy link
Member Author

kecnry commented Jan 16, 2019

@bpablo - can you check or remind us of the status of this? I know we changed the behavior slightly for 2.1 - was that enough to consider this closed or not?

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

No branches or pull requests

4 participants