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

dsavransky / EXOSIMS

Simulator for exoplanet direct imaging space missions

BSD 3-Clause "New" or "Revised" License

25 stars 35 forks source link

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

Open steigersg opened 5 months ago

steigersg commented 5 months ago

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

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

dsavransky commented 5 months ago

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 commented 5 months ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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.