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

added function to allow irisao command to be plotted in a subplot of … #275

Merged
merged 4 commits into from
May 10, 2022

Conversation

sfr348
Copy link
Contributor

@sfr348 sfr348 commented May 3, 2022

https://jira.stsci.edu/browse/CATKIT-103

currently it is very difficult to plot an irisao command, this function facilitates that. it does posses some re-used code from display(): so maybe we can make that cleaner by having display() call get_wavefront_custom()?

@sfr348 sfr348 requested a review from ehpor May 3, 2022 22:00
@sfr348 sfr348 self-assigned this May 3, 2022
@sfr348 sfr348 marked this pull request as draft May 3, 2022 22:01
@sfr348 sfr348 marked this pull request as ready for review May 3, 2022 22:06
@ivalaginja
Copy link
Contributor

Hey @sfr348, could you precise what this new function's intention is and how it differs from display()? It seems like it is doing exactly the same thing but without the actual plotting, and by getting rid of all plotting defaults. Is that the intended behavior? What is your use case?

@ivalaginja
Copy link
Contributor

Also, be mindful that you created this branch out of the tagged version, not develop. This shouldn't matter much in your case because the branch will need to be rebased on dev anyway after we fix the CI issue, but something to keep in mind for the future, it would save you some work.

@sfr348
Copy link
Contributor Author

sfr348 commented May 4, 2022

What is your use case?

So I wanted to be able to plot the irisAO command as a subplot in a figure post-experiment (like for making talk slides etc); I provided the sample code to do this in the description of get_wavefront_custom(). I could use display() to update the opd and just produce a throwaway plot but that seemed annoying. I just updated it to remove the duplicate code, I didn't want to mess with any of the base functions initially when I didn't have my own branch. I can also change the name of get_wavefront_custom() to something more intuitive

@ivalaginja
Copy link
Contributor

ivalaginja commented May 6, 2022

@sfr348 this looks good to me, however the name of the new function could be clearer, as you indicate yourself. Looking at what the new function does, it retrieves the current data of the segmented command in question and applies it to the aperture, which is a simulated proxy we use for plotting. So maybe you could rename the function into something like apply_current_data() or apply_current_wavefront() or something the like?

Apart from that, you'll have to rebase this branch after we merged some things recently into dev.

@sfr348 sfr348 force-pushed the feature/CATKIT-103-enable-irisao-subplot branch from f04839e to 7c0f4cf Compare May 6, 2022 18:59
@sfr348
Copy link
Contributor Author

sfr348 commented May 6, 2022

@ivalaginja all rebased against develop.

@sfr348 sfr348 requested a review from ivalaginja May 6, 2022 21:00
Copy link
Contributor

@ivalaginja ivalaginja left a comment

Choose a reason for hiding this comment

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

good by me

@ehpor ehpor merged commit 10db043 into develop May 10, 2022
@ehpor ehpor deleted the feature/CATKIT-103-enable-irisao-subplot branch May 10, 2022 02:22
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.

None yet

3 participants