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

Implement capping of catalog MAXMAG to 11.2 mag #301

Merged
merged 1 commit into from
Jun 11, 2019
Merged

Conversation

taldcroft
Copy link
Member

Based on discussions in emails ("maxmag" near June 7, 2019), the MAXMAG parameter in commanded star catalogs should be capped at 11.2 mag. This makes that change in proseco catalogs.

This PR does not implement capping of the maxmag parameter found in the acq star selection code. That value of maxmag is actually a different quantity, defined as 1-sigma above the star mag. In practice that will rarely be above 11.2 for selected stars, but in any case I would rather leave the acq selection algorithm unchanged so this PR does not introduce any change in star selection.

cc: @mbaski @jskrist

@taldcroft taldcroft requested a review from jeanconn June 10, 2019 20:56
@taldcroft taldcroft merged commit 1664cb8 into master Jun 11, 2019
@taldcroft taldcroft deleted the clip-maxmag branch June 11, 2019 16:13
@jeanconn
Copy link
Contributor

Should this just be cut as proseco 4.5? I don't think there are any doc or other action items to close out with regard to the new maxmag.

@taldcroft
Copy link
Member Author

I'm going to defer to your judgement as Ska release manager. I'm honestly not sure about the MATLAB release schedule, if this should go in to the Matlab release, or when, and etc. The handling of this is not entirely clear cut as you probably realized.

@jeanconn
Copy link
Contributor

OK. I think we should just cut this as a release, but I didn't consider the maxmag change as a bugfix. This would mean a 4.5 release; I think we haven't really decided if that means we need to do anything with 4.3.x and 4.4.x branches.

@jeanconn
Copy link
Contributor

Oh and I suppose we need to update the ACA load review checklist ACA-041 and also update the starcheck check to match .

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.

2 participants