Make plot() method behave consistently and correctly #221
Conversation
taldcroft
force-pushed
the
fix-plot
branch
2 times, most recently
from
095a712
to
0bda4c6
Compare
|
@jeanconn - related to your comment about different definitions of bad_stars, hopefully the new name and code comments help. I also realized it would make sense to set the |
| @@ -288,6 +290,26 @@ def process_include_ids(self, cand_guides, stars): | |||
|
|
|||
| super().process_include_ids(cand_guides, stars[ok]) | |||
|
|
|||
| def get_candidates_mask(self, stars): | |||
| """Get base filter for acceptable candidates. | |||
There was a problem hiding this comment.
Worth noting that, in general, I don't know what value the label of "acceptable" really has when the CCD-position and more detailed spoiler checks have not been done. But that's more of an issue for the seeking-the-right-roll PR.
There was a problem hiding this comment.
"Acceptable" means it can be chosen as a candidate. It doesn't mean it will be a candidate. Conversely, a star that is "not acceptable" will never be chosen as a guide or acq, so this is the initial entry ("base") filter.
| # internal version (which is no lot always the right answer). | ||
| kwargs.setdefault('bad_stars', np.zeros(len(kwargs['stars']), dtype=bool)) | ||
|
|
||
| plot_stars(attitude=self.att, ax=ax, **kwargs) |
There was a problem hiding this comment.
Looks good. Do we need more changes/options in chandra_aca.plot_stars? Other colors etc?
There was a problem hiding this comment.
Maybe, but that's for another day.
|
Thanks, and yeah, I don't know if we should just come up with a new name for the list of stars we are tracking that should be excluded, but the _mask helps some. |
| :param cand_guides: Table of candidate stars | ||
| :returns: boolean mask where True means star is in bad star list | ||
| """ | ||
| bad = np.in1d(cand_guides['id'], list(ACA.bad_star_set)) |
There was a problem hiding this comment.
I discovered this function recently and it's very handy! But apparently you cannot have a set as the second argument.
| """ | ||
| bad = np.in1d(cand_guides['id'], list(ACA.bad_star_set)) | ||
|
|
||
| # Set any matching bad stars as bad for plotting |
There was a problem hiding this comment.
Side effects like this are not totally ideal, but so it goes...
| """ | ||
| Obsid 17896 attitude | ||
| Will need to find another bad star, as this one is now excluded via VAR | ||
| def test_bad_star_list(): |
There was a problem hiding this comment.
Changed to a synthetic test, esp. since the comment indicated the test might not be valid anyway.
There was a problem hiding this comment.
I think this will make conflicts on #218 which I may let you resolve.
Plotting was kinda broken with regards to bad stars and the attempt to use just a single method for all classes didn't cut it. This PR fixes things up by introducing class-specific plot methods that do the right thing. Along the way I did a lot of refactoring and re-organizing:
bad_stars_maskpropertyget_candidates_maskmethod that indicates acceptable candidates (apart from spatial filtering)catalog_typeclass attribute to indicate the type of catalog represented by the class.get_catalog_for_plot(minor catalog munge for plotting)There was a reason for the madness, namely in development of #216 I was frustrated by the bugs and not being able to evaluate by eye whether a particular candidate should be acceptable at a different roll.
This supercedes #222.