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

Copy back the original kwargs into the monitor catalog call args #365

Merged
merged 2 commits into from
Jul 14, 2021

Conversation

jeanconn
Copy link
Contributor

Description

Copy back the original kwargs into the monitor catalog call args

The ACACatalogTable object should have the original arguments and not any arguments that were used just to determine if the monitor window should be a MON or a guide-from-mon, and this change assures that.

Testing

Fixes #364

@jeanconn jeanconn requested a review from taldcroft July 13, 2021 16:13
@jeanconn
Copy link
Contributor Author

jeanconn commented Jul 13, 2021

I don't see any reason that the catalog needs to have the "as solved-for" monitor kwargs instead of the original ones, but if there is another driver for the way this code was working, @taldcroft can speak to it. I also figure he may want to go with a different fix strategy, but this fix can be tested to at least see if I have the scope correctly isolated.

@taldcroft
Copy link
Member

Looks good, but just move each of the lines setting the kwargs to be immediately after the line where the catalog is created. That makes the code localized. Also add a comment like # Needed for roll optimization, see #364 in each case.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

Approved, assuming this has been tested with the new sparkles test.

@jeanconn
Copy link
Contributor Author

I confirm this was re-tested with the new sparkles test.

@jeanconn jeanconn merged commit c527e31 into master Jul 14, 2021
@jeanconn jeanconn deleted the fix-mon-kwargs branch July 14, 2021 16:04
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.

Issues with roll optimization of a catalog with monitor windows
2 participants