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

Add img_size guide parameter to control guide star image readout size #344

Merged
merged 6 commits into from
Sep 29, 2020

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Sep 28, 2020

Description

Add img_size parameter to control box size

Testing

  • Passes unit tests on Linux
  • Functional testing (test added)

Fixes #337

Requires:

  • API test with Matlab tools

proseco/catalog.py Outdated Show resolved Hide resolved
proseco/catalog.py Outdated Show resolved Hide resolved
@jeanconn jeanconn requested a review from taldcroft September 28, 2020 15:17
- Parameter name img_size_guide for API consistency
- Make img_size an attribute only for GuideCatalog
- Validate img_size value
- Faster test
@taldcroft
Copy link
Member

@jeanconn - I had a number of suggestions and in the end it was easier to just do them in code. I think this is ready so now you should review.

@taldcroft
Copy link
Member

taldcroft commented Sep 29, 2020

@jskrist - just want to confirm for MATLAB API that you have the machinery to include this optional parameter as necessary.

We are imagining (like the include/exclude interface) that there will be a text option box to manually set the guide star image readout size. It would normally be blank in which case ORviewer does not pass that parameter into the get_aca_catalog call and the Python default of None is used in proseco. But if a value is entered then that gets passed as an integer. Legal values are 4, 6, or 8.

@taldcroft taldcroft changed the title Add star_img_sz parameter to control box size Add img_size guide parameter to control guide star image readout size Sep 29, 2020
if value not in (6, 8, None):
raise ValueError('img_size must be 6, 8, or None')
if value not in (4, 6, 8, None):
raise ValueError('img_size must be 4, 6, 8, or None')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good. I was thinking this would get checked on the sparkles side but fine to do it here.

Copy link
Contributor Author

@jeanconn jeanconn left a comment

Choose a reason for hiding this comment

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

These changes look good to me. I like the idea of moving the parameter to be an attribute only on the guide catalog. Seems fine to add 4x4. And thanks for cleaning up the test by adding an empty dark map and token catalog to speed it up. I note I can't approve my own PR or apparently change owner, so this is just a comment.

@jeanconn
Copy link
Contributor Author

I note I just asked the question in sot/sparkles#147 .. should we set the OR default size somewhere in the proseco config?

@taldcroft
Copy link
Member

should we set the OR default size somewhere in the proseco config?

This is now coded in the GuideTable.get_img_size() method for any applications to use. Does that work?

@taldcroft taldcroft merged commit 68075c9 into master Sep 29, 2020
@taldcroft taldcroft deleted the specify_sz branch September 29, 2020 19:20
@javierggt javierggt mentioned this pull request Dec 7, 2020
@javierggt javierggt mentioned this pull request Apr 6, 2021
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.

Allow more control of guide readout size
2 participants