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

Multiply the "timeMultiplier" into the calculated integration time in Nemati OpticalSystem. #370

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

steigersg
Copy link

@steigersg steigersg commented Mar 13, 2024

Describe your changes

Add a line in the Nemati OpticalSystem to multiply the "timeMultiplier" into the calculated integration time. Added same line to the OpticalSystem and KasdinBraems OpticalSystem.

Also added a unit test to test_OpticalSystem.py to catch this for any existing or future OpticalSystems.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Reference any relevant issues (don't forget the #)

Fixes #369

Checklist before requesting a review

  • I have verified that all unit tests pass in a clean virtual environment and added new unit tests, as needed
  • I have run e2eTests and added new test scripts, as needed
  • I have verified that all docstrings are properly formatted and added new documentation, as needed

…on time in the calc_intTime function of the Nemati OpticalSystem
…ntegration time in the calc_intTime function of the Nemati OpticalSystem"

This reverts commit b28ba8a.
…ti OpticalSystem. Also added a small in line comment.
@dsavransky
Copy link
Owner

The pull looks good. I have one request: could you add a short unit test (at the prototype level) that catches intMultiplier regressions like this for all optical system modules?

@steigersg
Copy link
Author

The pull looks good. I have one request: could you add a short unit test (at the prototype level) that catches intMultiplier regressions like this for all optical system modules?

Added the unit test and in the process realized that the same issue was present in the OpticalSystem prototype and the KasdinBraems OpticalSystem as well. I pushed the same change to those calc_intTime functions but can revert those commits if you think it is out of the scope for this PR.

@CoreySpohn
Copy link
Collaborator

Coming back to this and wanted to make sure I remember right. The next step is making sure the schedulers are not double counting the time multiplier, correct? Just a quick scan in coroOnlyScheduler makes it look like the overhead time gets added to the characterization integration time before multiplying by timeMultiplier which makes me think we'd need to change that implementation (in next_target).

I also double checked and none of the files inside the Roman ETC have a timeMultiplier so it shouldn't impact that (wasn't sure how much we scrubbed those files before Sergi used them).

@dsavransky
Copy link
Owner

@CoreySpohn Correct - we basically want to unwind any use of the intMultiplier outside of the OpticalSystem - this should occur only in a handful of schedulers.

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.

timeMultiplier not respected in Nemati OpticalSystem
3 participants