Skip to content

Constraints - #61

Merged
bmorris3 merged 65 commits into
astropy:masterfrom
bmorris3:constraints
Sep 2, 2015
Merged

bmorris3 merged 65 commits into
astropy:masterfrom
bmorris3:constraints

Conversation

@bmorris3

Copy link
Copy Markdown
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
Copy Markdown
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

adrn commented Aug 13, 2015

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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?

@cdeil cdeil added this to the 0.2 release milestone Aug 17, 2015
Comment thread astroplan/constraints.py Outdated

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread docs/tutorials/constraints.rst Outdated

Copy link
Copy Markdown
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

cdeil commented Sep 1, 2015

Copy link
Copy Markdown
Member

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

cdeil commented Sep 1, 2015

Copy link
Copy Markdown
Member

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.

Comment thread docs/tutorials/constraints.rst Outdated

Copy link
Copy Markdown
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

eteq commented Sep 2, 2015

Copy link
Copy Markdown
Member

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

bmorris3 commented Sep 2, 2015

Copy link
Copy Markdown
Contributor Author

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

cdeil commented Sep 2, 2015

Copy link
Copy Markdown
Member

🎈 🎈 🎈 🎉 🎈 🎈 🎈

@eteq

eteq commented Sep 2, 2015

Copy link
Copy Markdown
Member

🎆

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.

6 participants