-
Notifications
You must be signed in to change notification settings - Fork 7
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
SCHED 407: Time accounting #280
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #280 +/- ##
==========================================
- Coverage 79.74% 79.43% -0.32%
==========================================
Files 64 64
Lines 3960 3978 +18
==========================================
+ Hits 3158 3160 +2
- Misses 802 818 +16
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small changes and a question.
I'm not fully sure this is the right logic, but apart from the ONGOING, it looks like what Bryan is doing, so let's go with this.
# check that Observation is Observed | ||
observation.status = ObservationStatus.ONGOING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit confused about this... why are we always setting to ONGOING
?
I think we need to check if the sequence is complete and set to OBSERVED
if it is, and ONGOING
if it isn't, like here:
if atom_end == seq_length: |
atom.program_used = atom.prog_time | ||
atom.partner_used = atom.part_time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is right... we assume they were completed, so we use up all their time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is close but won't work correctly when the observation is split. At the moment the entire sequence is set to OBSERVED and charged, even if it should should be split. If only first n atoms are to be observed, then , then only the first 5 atoms are updated. If the first atom to be observed is not the first one (index 0), then the loop must start in the middle of the sequence.
I recall now, looking at the plan classes, that the Visit class has field for the starting/ending atom indices but they are currently set to the first and last atoms not matter what. This is something that I forgot to update when adding splitting. We need to add atom_start/end_idx variables to the plan.add method call and then update greedymax.output_plans to pass them. Then the loop can go from v.atom_start_idx to v.atom_end_idx.
if atom_idx == 0: | ||
if observation.obs_class == ObservationClass.PARTNERCAL: | ||
atom.program_used += observation.acq_overhead | ||
elif (observation.obs_class == ObservationClass.SCIENCE or | ||
observation.obs_class == ObservationClass.PROGCAL): | ||
atom.program_used += observation.acq_overhead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing this, you should just be able to move this out of the for loop and do it directly for first_atom = observation.sequence[0]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is the for has to be changed as well because it would replace the values in the first iteration and if the first atom is skip entirely then now we have to add the first atom.progtime as well. I guess is just matter of personal preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if atom_idx == 0 should be if atom_idx = v.atom_start_idx.
atom.observed = True | ||
atom.qa_state = QAState.PASS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge it!
No description provided.