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

Tutorial for Scheduling #218

Merged
merged 30 commits into from Aug 18, 2016
Merged

Conversation

kvyh
Copy link
Contributor

@kvyh kvyh commented Aug 5, 2016

This contains the Scheduling Tutorial in the docs, but was rebased off of code that it requires to function. This will replace #208 and is intended to solve #200 .

There is a work left to do on the tutorial itself so that it better reflects how it would be used, so I will be rewriting specific parts of it over the coming days.

@kvyh kvyh mentioned this pull request Aug 5, 2016
@bmorris3
Copy link
Contributor

@kvyh You'll have to rebase this branch.

@kvyh
Copy link
Contributor Author

kvyh commented Aug 10, 2016

The conflict was in a part of the code that I no longer needed, so I removed it.

@kvyh
Copy link
Contributor Author

kvyh commented Aug 10, 2016

It looks like the commit fixed the issue that was occurring with numpy1.7 (I don't really understand why), but the checks for this branch are running very slowly. Is there any way to help with the speed?

@@ -21,70 +22,182 @@
dec=89.26410897 * u.deg), name="Polaris")

apo = Observer.at_site('apo')
targets = [vega, rigel, polaris]
targets = [vega, polaris, rigel]
default_time = Time('2016-02-06 03:00:00')
Copy link
Contributor

@bmorris3 bmorris3 Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you call this "tricky_time" or something that indicates that this is a time where float evaluations go bad, which you're testing specifically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That default time is used in all of the tests, not just the insert_slot, would it be better to create the Time object separately for each test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably better to move the time into each test, because ideally in the future that will be updated to test multiple times (having only one test time is a pretty narrow test).

@bmorris3
Copy link
Contributor

Does this replace #203?

@bmorris3 bmorris3 self-assigned this Aug 11, 2016
@@ -683,7 +683,7 @@ def _make_schedule(self, blocks):
end_idx = times_indices + start_idx
# this may make some OBs get sub-optimal scheduling, but it closes gaps
# TODO: determine a reasonable range inside which it gets shifted
if (tb.duration > new_start_time - tb.start_time or
if (new_start_time - tb.start_time < tb.duration or
Copy link
Contributor

@bmorris3 bmorris3 Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lolwut ❓

@kvyh kvyh mentioned this pull request Aug 11, 2016
@bmorris3
Copy link
Contributor

Hey @bsipocz – some doctests are failing when we try to import matplotlib in the docs now. Could that be a result of any of the recent CI changes? Just want to check with you before I do some real digging.

@bsipocz
Copy link
Member

bsipocz commented Aug 12, 2016

@bmorris3 - According to the docs, mpl is optional dependency only, so no wonder the tests fail when it is not available.

@kvyh
Copy link
Contributor Author

kvyh commented Aug 12, 2016

That can easily be fixed by just skipping the doctests for it, like the plotting tutorial does.

@bmorris3
Copy link
Contributor

@kvyh Doctests are useful and should be run whenever we can run them.

@bsipocz Is there a way to ensure that matplotlib is installed for the doctests, without requiring matplotlib?

@bsipocz
Copy link
Member

bsipocz commented Aug 12, 2016

There is __doctest_requires__ where you can list the functions with
requirements as dictionary and also doctest-requires:: matplotlib for
docs files. There should be many examples in astropy core for the exact
usage.

On 12 August 2016 at 20:01, Brett M. Morris notifications@github.com
wrote:

@kvyh https://github.com/kvyh Doctests are useful and should be run
whenever we can run them.

@bsipocz https://github.com/bsipocz Is there a way to ensure that
matplotlib is installed for the doctests, without requiring matplotlib?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#218 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGeUwmehPGoMvPBYcLFclKshyEvo9F2qks5qfMMEgaJpZM4JdPew
.

@bmorris3
Copy link
Contributor

bmorris3 commented Aug 15, 2016

Reminder to rebase this branch after the changes in #219 were merged.

@kvyh
Copy link
Contributor Author

kvyh commented Aug 15, 2016

I rebased this and changed the tests to use the new _call_ for the schedulers.

@bmorris3
Copy link
Contributor

I'm currently trying to build the sphinx docs from your tutorial, and they're taking forever. Either there's a problem with the code in there, or the code is way too slow.

@kvyh
Copy link
Contributor Author

kvyh commented Aug 17, 2016

I have added the suggestions and rebased on the newest changes, it is much faster to build now.

@bmorris3
Copy link
Contributor

Thanks @kvyh! This is a big one.

@bmorris3 bmorris3 merged commit b5e7cfb into astropy:master Aug 18, 2016
@bmorris3 bmorris3 mentioned this pull request Aug 18, 2016
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.

None yet

3 participants