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

Constraints #61

Merged
merged 65 commits into from
Sep 2, 2015
Merged

Constraints #61

merged 65 commits into from
Sep 2, 2015

Conversation

bmorris3
Copy link
Contributor

Here's a first implementation of the constraints module, based on a rough pseudocode sketch for the inheritance scheme by @eteq.

To see how this constraints module would be used, take a peek at this demo notebook. UPDATE: I've added high level docs for you to inspect also.

This PR is intended to be a conversation starter. I've made some decisions in naming that will likely be controversial, so let's discuss!

@bmorris3
Copy link
Contributor Author

@eteq – The travis build failed on some Python 3 runs because Time is not a hashable type. Any idea why this would affect py3 but not py2? Should I make a tuple of the JDs instead?

@adrn
Copy link
Member

adrn commented Aug 13, 2015

Nice! I'm digging this.

I was talking to @astrofrog earlier about a use case that would be nice to have: given an observer, a single time, a list of targets, constraints, and (optionally) a list of exposure times (or is this an attribute of Target?), which targets can I observe? This would be super nice to have when observing -- not for scheduling, but for more free form observing :).

Amateur astronomers would probably also find this useful -- we could imagine building a way to, given a catalog name, return all objects that are visible (e.g., what Messier objects are observable right now from XX?) Thinking a bit more long term, it would also be straightforward to build a web front-end with Flask and yadda yadda...

@jberlanga
Copy link
Member

This looks good! I like the is_observable/is_always_observable methods in particular--exactly the questions I was trying to answer in my Summer Triangle tutorial.

@bmorris3
Copy link
Contributor Author

A wishlist of constraints that could/should be added is developing on my wiki. Contribute if there's anything missing!

@astroplanners/astroplanners-contributors: @jberlanga brought up one type of constraint that we haven't discussed in a while – target-intrinsic constraints. For example: give me all targets in this list with 11<V<14. Of course, by themselves these target-intrinsic constraints could be sorted out by the user in a simple for loop before they hit the constraints module, but it would be more natural to write them into a subclass of constraint.

One option for doing this could be to allow users to add their own kwargs to FixedTarget which gets stored on the object, and then have a generic TargetPropertyConstraint(property_keyword, [min, max]) that loops through the targets and applies the cut by the supplied keyword. That might look like this:

from astroplan import FixedTarget
from astroplan.constraints import Constraint

polaris = FixedTarget.from_name("Polaris", Bmag=2.62)
aldebaran = FixedTarget.from_name("Aldebaran", Bmag=2.4)
algol = FixedTarget.from_name("Algol", Bmag=2.07)

class TargetPropertyConstraint(Constraint):
    def __init__(self, property_keyword, min=None, max=None):
        self.min = min
        self.max = max
        self.property_keyword = property_keyword

    def _compute_constraint(self, time_range, observer, targets):
        if self.min is None:
            mask = [self.min < target[self.property_keyword]
                    for target in targets]
        elif self.max is None:
            mask = [self.max > target[self.property_keyword]
                    for target in targets]
        elif self.min is not None and self.max is not None:
            mask = [self.min < target[self.property_keyword]
                    for target in targets] & [self.max > target[self.property_keyword]
                    for target in targets]
        return mask

How would you like to see this?

def is_always_observable(constraints, time_range, targets, observer):
"""
Are the ``targets`` always observable throughout ``time_range`` given
constraints in ``constraints_list`` for ``observer``?
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like "constraints_list" should be changed to "constraints" to match the code?

@cdeil cdeil modified the milestones: 0.1 release, 0.2 release Aug 25, 2015
@bmorris3
Copy link
Contributor Author

@cdeil and others: there are some high level docs for the constraints module here. I'll make a commit with those docs included in the PR once @jberlanga 's high level docs get posted (#42), so I can link to the tutorial from her tutorials page.

write must be as well.

In this example, let's design our constraint to ensure that all targets must be
within some number of degrees from Vega – we'll call it
Copy link
Member

Choose a reason for hiding this comment

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

"number of degrees" -> "angular separation"?

@cdeil
Copy link
Member

cdeil commented Sep 1, 2015

I've left a bunch of inline comments.

Two more big-picture comments:

  • Is the user-defined constraint tested via doctest? If no, do you think it's worth copying over to test_constraints.py (with a note that it is the example from the docs)?
  • I think the constraints section can stand on it's own, it's nice! But I'd add a sentence at the start, saying something like "this is a start ... more high-level functionality to compute observability and do scheduling will be added to astroplan soon ... feature requests and contributions welcome!"

@cdeil
Copy link
Member

cdeil commented Sep 1, 2015

I forgot to say: IMO this is nice and it's been open for a while, so @bmorris3, if you want to merge it, I'd say go ahead! Or leave it open for one more day and ping the others for final review one more time, as you like.

@@ -0,0 +1,255 @@
:orphan:
Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessary - it was needed in the past when there was no index pointing to the tutorials, but now there is, so you should not use :orphan: (it indicates that the document in question isn't linked to from anywhere else).

@eteq
Copy link
Member

eteq commented Sep 2, 2015

Ok, I'm done with my review - mostly doc suggestions (although one possibly-complicated bit about user-defined constraints... we may want to discuss that more on slack)

This has already been discussed some on slack, but I think the whole constraints tutorial should be doctest-ed. Annoyingly the easiest way is probably to add >>> in front of basically everything. But if we don't want to have to figure out how to do this right for 0.1 it could also be moved to the test suite as @cdeil suggested.

@bmorris3
Copy link
Contributor Author

bmorris3 commented Sep 2, 2015

Updates done, tests passed, merging! Feel free to leave more comments in issues if I jumped the gun on anything.

bmorris3 added a commit that referenced this pull request Sep 2, 2015
@bmorris3 bmorris3 merged commit b750f8a into astropy:master Sep 2, 2015
@cdeil
Copy link
Member

cdeil commented Sep 2, 2015

🎈 🎈 🎈 🎉 🎈 🎈 🎈

@eteq
Copy link
Member

eteq commented Sep 2, 2015

🎆

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

Successfully merging this pull request may close these issues.

None yet

6 participants