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

Legacy wrapper support for exposure times #69

Open
kecnry opened this issue Sep 4, 2016 · 5 comments
Open

Legacy wrapper support for exposure times #69

kecnry opened this issue Sep 4, 2016 · 5 comments
Assignees
Milestone

Comments

@kecnry
Copy link
Member

kecnry commented Sep 4, 2016

Legacy wrapper (as well as importer and exporter) should support finite exposure times.

For the legacy wrapper, this may mean making multiple calls to legacy since legacy only supports a single exposure time across all datasets whereas phoebe2 now supports per-LC-dataset exposure times and options (as of aca3115).

Alternately, we could easily allow phoebe2 to handle the oversampling and just request the oversampled LC and then average. In this case the exporter should behave differently when not called by the wrapper and still attempt to export the exposure times. The only downside to this approach is that we can't directly test oversampling vs legacy. The big upside is that it can just as easily be applied to all other backends.

@kecnry kecnry added this to the 2.0b milestone Sep 4, 2016
@bpablo
Copy link
Contributor

bpablo commented Sep 17, 2016

I have run into an issue in the deployment of this. Phoebe 1's etc consists of 4 parameters:
1.) on/off
2.) integration time
3.) oversampling
4.) time taken from (i.e. midpoint, start, end)

Integration time is easy to add. I can't find a switch so is etc always on in phoebe2? What about 2 and 3? do these exist somewhere? I have tried looking through the bundle and the website but i haven't found anything.

@kecnry
Copy link
Member Author

kecnry commented Sep 17, 2016

1.) In phoebe2 there are exposure/integration times per-dataset (see qualifier='exptime', context='dataset').

2.) There are switches in the compute options per-dataset (feel free to do the same for the legacy compute options). For phoebe2 this is a choice parameter (see qualifier='fti_method', context='compute')... so I think it may make sense to copy that and only have two options: ['none', 'oversampling'] rather than create a BoolParameter.

3.) You'll also see an IntegerParameter for the number of samples (see qualifier='fti_oversample', context='compute').

4.) We only support midpoint in phoebe2, so its safe to assume that in phoebe2->phoebe1, but for phoebe1->phoebe2 raise a warning if they're not set at midpoint.

@kecnry kecnry modified the milestones: 2.0, 2.0b Sep 26, 2016
@bpablo
Copy link
Contributor

bpablo commented Jan 25, 2017

@aprsa @kecnry . I am about halfway done with this, but there is one issue which I have not raised with you yet. That is that now phoebe1 accepts exposure time for all lc and rv datasets. Phoebe 2 only accepts exposure times for light curves. I see no reason why phoebe2 wouldn't also have exposure times for RVs, but if they exist then the logic is different than for lcs.

kecnry added a commit that referenced this issue Jan 25, 2017
@kecnry
Copy link
Member Author

kecnry commented Jan 25, 2017

@bpablo - see commit 92db431 for a fix to the visibility of fti_oversample

As for RV datasets - I believe we (Andrej and I) talked about this and decided to defer since it is more complicated than just oversampling in time (probably need to weigh by instantaneous intensities?). So unless we want to handle that now, set oversampling to 0 for RVs when doing phoebe2 -> phoebe1 and ignore/warn when doing phoebe1 -> phoebe2.

bpablo added a commit that referenced this issue Feb 1, 2017
Legacy wrapper now lets the user input finite exposure times, but throws a warning when sent to the legacy wrapper and turns computation off. relates to issue #69
@kecnry
Copy link
Member Author

kecnry commented Feb 1, 2017

I'm deferring exposure times via legacy wrapper for a future release. We'll consider this closed for 2.0 and revisit the rest later.

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

No branches or pull requests

2 participants