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

Move simple simulator and model from hicat-package2 to catkit2 #192

Merged
merged 10 commits into from
May 18, 2024

Conversation

ivalaginja
Copy link
Collaborator

@ivalaginja ivalaginja commented Apr 24, 2024

Fixes #187.

Pair PR with https://github.com/spacetelescope/hicat-package2/pull/587.

Needs testing and converting the added test into a real test that pytest understands.

@ivalaginja ivalaginja requested a review from ehpor April 24, 2024 16:33
@ivalaginja ivalaginja self-assigned this Apr 24, 2024
@ivalaginja ivalaginja marked this pull request as draft April 24, 2024 16:35
@ivalaginja ivalaginja force-pushed the feature/move_simple_simulator_and_model branch from 1db9c0f to 501fda9 Compare April 25, 2024 09:03
@ivalaginja ivalaginja removed the request for review from ehpor April 25, 2024 15:27
@ivalaginja ivalaginja force-pushed the feature/move_simple_simulator_and_model branch from 08cdc89 to 0e40700 Compare May 1, 2024 20:00
@ivalaginja ivalaginja requested a review from ehpor May 1, 2024 20:17
@ivalaginja
Copy link
Collaborator Author

This is ready for review, although I don't really know how to make the test make more sense, and whether it's worth it.

@ivalaginja ivalaginja marked this pull request as ready for review May 1, 2024 20:17
size1 = test_wf.intensity.shaped.shape[0]
size2 = test_wf.intensity.shaped.shape[1]

stream = DataStream.create('images', 'mod', 'float64', [size1, size2], 20)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure why the data stream is needed here. This is a leftover from very early on, when I wanted to test an early version of the camera viewer UI.

@ehpor
Copy link
Collaborator

ehpor commented May 1, 2024

This is ready for review, although I don't really know how to make the test make more sense, and whether it's worth it.

The test looks fine to me. Basic functionality test and checks that the code doesn't crash.

@ivalaginja ivalaginja requested a review from ehpor May 2, 2024 13:09
@ivalaginja ivalaginja force-pushed the feature/move_simple_simulator_and_model branch from d2f435e to 68a1b62 Compare May 2, 2024 18:00
@ivalaginja ivalaginja force-pushed the feature/move_simple_simulator_and_model branch from 29d5f02 to e987286 Compare May 16, 2024 16:16
Copy link
Collaborator

@raphaelpclt raphaelpclt left a comment

Choose a reason for hiding this comment

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

Looks good overall.
Just a few small questions, nothing serious

catkit2/simulator/simple_optical_model.py Show resolved Hide resolved
tests/test_optical_model.py Show resolved Hide resolved
@ivalaginja ivalaginja force-pushed the feature/move_simple_simulator_and_model branch from e987286 to a446d9d Compare May 18, 2024 06:35
@ivalaginja
Copy link
Collaborator Author

@raphaelpclt all fair questions, I hope I answered them sufficiently, ready for a new review. Let me know if you'd need more.

Copy link
Collaborator

@raphaelpclt raphaelpclt left a comment

Choose a reason for hiding this comment

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

Looks good

@ivalaginja ivalaginja merged commit 6c8900a into develop May 18, 2024
6 checks passed
@ivalaginja ivalaginja deleted the feature/move_simple_simulator_and_model branch May 18, 2024 11:36
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.

Move simple simulator and optical model into catkit2
3 participants