-
Notifications
You must be signed in to change notification settings - Fork 25
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
Make API for arbitrary shapes (other than transits) #2
Comments
Hi. I've already made an implementation for this (based on the exocomets proposal) in my TLS fork. Do you want to check it? Maybe if you found it as a good solution (which might be extensible to any other transit templates), I could create a Pull Request to merge it into the official TLS repository. If you also think that the solutions is not good enough yet, please let me know what additional changes could I do to step forward towards a Pull Request. Kind regards. |
Cool! I'm very interested in adding this to the main branch of TLS. |
Great. I'll come back with a good explanation, a notebook containing examples and some documentation about the code as soon as I can. Do you have any prefered format for the documentation? |
Wonderful! You could build on the existing documentation (source, ReadTheDocs) and link an iPython notebook? |
Great. I will do so. I'll let you know when I'm finished. |
I think that I have added a good start to the documentation of the feature in both places. Please have a look at the notebooks that I added and to the light api changes and let me know whether you think that I should add more info or modify the one I'm providing. In general terms, I have moved the template related logic to a class named TransitTemplateGenerator, which is implemented by DefaultTransitTemplateGenerator for planetary transits. There is also CometTransitTemplateGenerator now. Finally, the user can decide to provide his custom implementation of TransitTemplateGenerator to search for custom templates. You can see in the last notebook that I tried a template for flares. I had to modify tls logic which assumed that the templates only caused flux drops, and now flux increments would be supported. The main issue I'm concerned about right now is the transit depth adjustment, which can be inexact for several reasons. One of them is the overshoot parameter, whose meaning I dont understand. I think that you created an issue to provide depth fits instead of an analytical one and that might be related too. However I think that the solution works properly enough now and the things that I'm commenting here could probably be developed in the future. Kind regards. |
As soon as I can I will create some tests and will open a pull request so you can better inspect the changes. |
Sorry @martindevora again for the delay. Schools are still closed which leaves me hardly any time. I hope the situation will get better in February. Current estimate is either 1 Feb or 15 Feb for re-opening. |
There is no hurry at all @hippke . I'm adding a few improvements into the new API so we can have a more maintainable code. The most important thing right now is to keep your family and yourself safe and healthy. I'm doing as well. I would probably like to use these changes as a base to develop some kind of publication at some point in the future and I would like to know what you think about it. As already said, there is no hurry, so don't worry about this until you feel like you can do it happily without pressure. |
I think it's a great idea worth implementing in code, and worth searching for in real data. I'm not a specialst of flare-finding algorithms, but the few I have seen are rather simple. So it appears useful to try another approach. |
Yes totally. I'm planning to compare TLS against such other algorithms (with its improvements and counterparts) and with the standard transit models in case of tailed transits. I was also wondering in using a final_fit method within TLS to add the option to the user of adding a complete custom fit for the given template, which could be a scipy.curve_fit for instance (or whatever the user would like to introduce). What do you think about it? I guess that it is not strictly necessary as it can be done after TLS returns the results, but it would be a way to unify the final fit processing for all the search algorithms. Kind regards. |
e.g., flares
The text was updated successfully, but these errors were encountered: