Merge pull request #99 from LCOGT/fix_fill_window

switch the fill_window attribute to act on the configuration instead …

observation_portal/requestgroups/migrations/0013_auto_20191220_0711.py

@@ -0,0 +1,24 @@
+# Generated by Django 2.2.4 on 2019-12-20 07:11
+
+import django.core.validators
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('requestgroups', '0012_auto_20191123_0233'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='configuration',
+            name='repeat_duration',
+            field=models.FloatField(blank=True, help_text='The requested duration for this configuration to be repeated within. Only applicable to REPEAT_* type configurations. Setting parameter fill_window to True will cause this value to automatically be filled in to the max possible given its visibility within the observing window.', null=True, validators=[django.core.validators.MinValueValidator(0.0)], verbose_name='configuration duration'),
+        ),
+        migrations.AlterField(
+            model_name='instrumentconfig',
+            name='exposure_count',
+            field=models.PositiveIntegerField(help_text='The number of exposures to take. This field must be set to a value greater than 0.', validators=[django.core.validators.MinValueValidator(1)]),
+        ),
+    ]

observation_portal/requestgroups/models.py

@@ -370,7 +370,9 @@ class Configuration(models.Model):
         null=True,
         validators=[MinValueValidator(0.0)],
         help_text='The requested duration for this configuration to be repeated within. '
-                  'Only applicable to REPEAT_* type configurations.'
+                  'Only applicable to REPEAT_* type configurations. Setting parameter fill_window '
+                  'to True will cause this value to automatically be filled in to the max '
+                  'possible given its visibility within the observing window.'
     )
 
     extra_params = JSONField(
@@ -618,11 +620,7 @@ class InstrumentConfig(models.Model):
     )
     exposure_count = models.PositiveIntegerField(
         validators=[MinValueValidator(1)],
-        # TODO: Update help text. Maybe move fill window instructions to a better place and improve them.
-        help_text='The number of exposures to take. This field must be set to a value greater than 0, but optionally '
-                  'you can add a boolean fill_window field to the Configuration upon submission with a value '
-                  'of true. If this is set, the exposure_count will be set to the number of exposures (including '
-                  'overheads) that will fit in the observing window.'
+        help_text='The number of exposures to take. This field must be set to a value greater than 0.'
     )
     bin_x = models.PositiveSmallIntegerField(
         verbose_name='y binning', default=1, blank=True,

observation_portal/requestgroups/serializers.py

@@ -144,7 +144,6 @@ def validate(self, data):
 
 
 class InstrumentConfigSerializer(serializers.ModelSerializer):
-    fill_window = serializers.BooleanField(required=False, write_only=True)
     rois = RegionOfInterestSerializer(many=True, required=False)
 
     class Meta:
@@ -214,6 +213,7 @@ def validate(self, data):
 
 
 class ConfigurationSerializer(serializers.ModelSerializer):
+    fill_window = serializers.BooleanField(required=False, write_only=True)
     constraints = ConstraintsSerializer()
     instrument_configs = InstrumentConfigSerializer(many=True)
     acquisition_config = AcquisitionConfigSerializer()
@@ -234,8 +234,6 @@ def to_representation(self, instance):
         return data
 
     def validate_instrument_configs(self, value):
-        if [instrument_config.get('fill_window', False) for instrument_config in value].count(True) > 1:
-            raise serializers.ValidationError(_('Only one instrument_config can have `fill_window` set'))
         if len(set([instrument_config.get('rotator_mode', '') for instrument_config in value])) > 1:
             raise serializers.ValidationError(_('Rotator modes within the same configuration must be the same'))
         return value
@@ -549,6 +547,10 @@ def validate_configurations(self, value):
         if not value:
             raise serializers.ValidationError(_('You must specify at least 1 configuration'))
 
+        # Only one configuration can have the fill_window attribute set
+        if [config.get('fill_window', False) for config in value].count(True) > 1:
+            raise serializers.ValidationError(_('Only one configuration can have `fill_window` set'))
+
         constraints = value[0]['constraints']
         # Set the relative priority of molecules in order
         for i, configuration in enumerate(value):
@@ -615,22 +617,16 @@ def validate(self, data):
             rise_set_intervals_by_site = get_filtered_rise_set_intervals_by_site(data, is_staff=is_staff)
             largest_interval = get_largest_interval(rise_set_intervals_by_site)
             for configuration in data['configurations']:
-                for instrument_config in configuration['instrument_configs']:
-                    if instrument_config.get('fill_window'):
-                        instrument_config_duration = get_instrument_configuration_duration(
-                            instrument_config, configuration['instrument_type']
-                        )
-                        num_exposures = get_num_exposures(
-                            instrument_config, configuration['instrument_type'],
-                            largest_interval - timedelta(seconds=duration - instrument_config_duration)
-                        )
-                        instrument_config['exposure_count'] = num_exposures
-                        duration = get_request_duration(data)
-                    # delete the fill window attribute, it is only used for this validation
-                    try:
-                        del instrument_config['fill_window']
-                    except KeyError:
-                        pass
+                if 'REPEAT' in configuration['type'].upper() and configuration.get('fill_window'):
+                    max_configuration_duration = largest_interval.total_seconds() - duration + configuration.get('repeat_duration', 0) - 1
+                    configuration['repeat_duration'] = max_configuration_duration
+                    duration = get_request_duration(data)
+
+                # delete the fill window attribute, it is only used for this validation
+                try:
+                    del configuration['fill_window']
+                except KeyError:
+                    pass
             if largest_interval.total_seconds() <= 0:
                 raise serializers.ValidationError(
                     _(

observation_portal/requestgroups/test/test_api.py

@@ -1605,63 +1605,64 @@ def test_configurations_automatically_have_priority_set(self):
         for i, configuration in enumerate(rg['requests'][0]['configurations']):
             self.assertEqual(configuration['priority'], i + 1)
 
-    def test_fill_window_on_more_than_one_instrument_config_fails(self):
+    def test_fill_window_on_more_than_one_configuration_fails(self):
         bad_data = self.generic_payload.copy()
-        bad_data['requests'][0]['configurations'][0]['instrument_configs'].append(
-            self.extra_configuration['instrument_configs'][0].copy()
+        bad_data['requests'][0]['configurations'][0]['type'] = 'REPEAT_EXPOSE'
+        bad_data['requests'][0]['configurations'][0]['repeat_duration'] = 250
+        bad_data['requests'][0]['configurations'].append(
+            bad_data['requests'][0]['configurations'][0].copy()
         )
-        bad_data['requests'][0]['configurations'][0]['instrument_configs'][0]['fill_window'] = True
-        bad_data['requests'][0]['configurations'][0]['instrument_configs'][1]['fill_window'] = True
+        bad_data['requests'][0]['configurations'][0]['fill_window'] = True
+        bad_data['requests'][0]['configurations'][1]['fill_window'] = True
         response = self.client.post(reverse('api:request_groups-list'), data=bad_data)
-        self.assertIn('Only one instrument_config can have `fill_window` set', str(response.content))
+        self.assertIn('Only one configuration can have `fill_window` set', str(response.content))
         self.assertEqual(response.status_code, 400)
 
     def test_fill_window_one_configuration_fills_the_window(self):
         good_data = self.generic_payload.copy()
-        good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['fill_window'] = True
-        initial_exposure_count = good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['exposure_count']
-        response = self.client.post(reverse('api:request_groups-list'), data=good_data)
-        rg = response.json()
-        self.assertGreater(rg['requests'][0]['configurations'][0]['instrument_configs'][0]['exposure_count'],
-                           initial_exposure_count)
-        self.assertEqual(response.status_code, 201)
-
-    def test_fill_window_two_instrument_configs_one_false_fills_the_window(self):
-        good_data = self.generic_payload.copy()
-        good_data['requests'][0]['configurations'][0]['instrument_configs'].append(
-            self.extra_configuration['instrument_configs'][0].copy()
-        )
-        good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['fill_window'] = True
-        good_data['requests'][0]['configurations'][0]['instrument_configs'][1]['fill_window'] = False
-        initial_exposure_count = good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['exposure_count']
+        initial_repeat_duration = 250
+        good_data['requests'][0]['configurations'][0]['type'] = 'REPEAT_EXPOSE'
+        good_data['requests'][0]['configurations'][0]['repeat_duration'] = initial_repeat_duration
+        good_data['requests'][0]['configurations'][0]['fill_window'] = True
         response = self.client.post(reverse('api:request_groups-list'), data=good_data)
         rg = response.json()
-        self.assertGreater(rg['requests'][0]['configurations'][0]['instrument_configs'][0]['exposure_count'],
-                           initial_exposure_count)
+        self.assertGreater(rg['requests'][0]['configurations'][0]['repeat_duration'],
+                           initial_repeat_duration)
         self.assertEqual(response.status_code, 201)
 
-    def test_fill_window_two_instrument_configs_one_blank_fills_the_window(self):
+    def test_fill_window_two_configs_one_false_fills_the_window(self):
         good_data = self.generic_payload.copy()
-        good_data['requests'][0]['configurations'][0]['instrument_configs'].append(
-            self.extra_configuration['instrument_configs'][0].copy()
+        initial_repeat_duration = 250
+        good_data['requests'][0]['configurations'][0]['type'] = 'REPEAT_EXPOSE'
+        good_data['requests'][0]['configurations'][0]['repeat_duration'] = initial_repeat_duration
+        good_data['requests'][0]['configurations'].append(
+            good_data['requests'][0]['configurations'][0].copy()
         )
-        good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['fill_window'] = True
-        initial_exposure_count = good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['exposure_count']
+        good_data['requests'][0]['configurations'][0]['fill_window'] = True
+        good_data['requests'][0]['configurations'][1]['fill_window'] = False
         response = self.client.post(reverse('api:request_groups-list'), data=good_data)
         rg = response.json()
-        self.assertGreater(rg['requests'][0]['configurations'][0]['instrument_configs'][0]['exposure_count'], initial_exposure_count)
+        self.assertGreater(rg['requests'][0]['configurations'][0]['repeat_duration'],
+                           initial_repeat_duration)
+        self.assertEqual(rg['requests'][0]['configurations'][1]['repeat_duration'],
+                         initial_repeat_duration)
         self.assertEqual(response.status_code, 201)
 
-    def test_fill_window_two_instrument_configs_first_fills_the_window(self):
+    def test_fill_window_two_configs_one_blank_fills_the_window(self):
         good_data = self.generic_payload.copy()
-        good_data['requests'][0]['configurations'][0]['instrument_configs'].append(
-            self.extra_configuration['instrument_configs'][0].copy()
+        initial_repeat_duration = 250
+        good_data['requests'][0]['configurations'][0]['type'] = 'REPEAT_EXPOSE'
+        good_data['requests'][0]['configurations'][0]['repeat_duration'] = initial_repeat_duration
+        good_data['requests'][0]['configurations'].append(
+            good_data['requests'][0]['configurations'][0].copy()
         )
-        good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['fill_window'] = True
-        initial_exposure_count = good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['exposure_count']
+        good_data['requests'][0]['configurations'][0]['fill_window'] = True
         response = self.client.post(reverse('api:request_groups-list'), data=good_data)
         rg = response.json()
-        self.assertGreater(rg['requests'][0]['configurations'][0]['instrument_configs'][0]['exposure_count'], initial_exposure_count)
+        self.assertGreater(rg['requests'][0]['configurations'][0]['repeat_duration'],
+                           initial_repeat_duration)
+        self.assertEqual(rg['requests'][0]['configurations'][1]['repeat_duration'],
+                         initial_repeat_duration)
         self.assertEqual(response.status_code, 201)
 
     def test_fill_window_not_enough_time_fails(self):
@@ -1670,52 +1671,29 @@ def test_fill_window_not_enough_time_fails(self):
             'start': '2016-09-29T21:12:18Z',
             'end': '2016-09-29T21:21:19Z'
         }
-        bad_data['requests'][0]['configurations'][0]['instrument_configs'][0]['fill_window'] = True
+        bad_data['requests'][0]['configurations'][0]['type'] = 'REPEAT_EXPOSE'
+        bad_data['requests'][0]['configurations'][0]['repeat_duration'] = 250
+        bad_data['requests'][0]['configurations'][0]['fill_window'] = True
         response = self.client.post(reverse('api:request_groups-list'), data=bad_data)
         self.assertIn('the target is never visible within the time window', str(response.content))
         self.assertEqual(response.status_code, 400)
 
     def test_fill_window_confined_window_fills_the_window(self):
         good_data = self.generic_payload.copy()
+        initial_repeat_duration = 250
         good_data['requests'][0]['windows'][0] = {
             'start': '2016-09-29T23:12:18Z',
             'end': '2016-09-29T23:21:19Z'
         }
-        good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['fill_window'] = True
-        response = self.client.post(reverse('api:request_groups-list'), data=good_data)
-        rg = response.json()
-        self.assertEqual(rg['requests'][0]['configurations'][0]['instrument_configs'][0]['exposure_count'], 3)
-        self.assertEqual(response.status_code, 201)
-
-    def test_fill_window_confined_window_2_fills_the_window(self):
-        good_data = self.generic_payload.copy()
-        good_data['requests'][0]['windows'][0] = {
-            'start': '2016-09-29T23:12:18Z',
-            'end': '2016-09-29T23:21:19Z'
-        }
-        good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['exposure_time'] = 50
-        good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['fill_window'] = True
+        good_data['requests'][0]['configurations'][0]['type'] = 'REPEAT_EXPOSE'
+        good_data['requests'][0]['configurations'][0]['repeat_duration'] = initial_repeat_duration
+        good_data['requests'][0]['configurations'][0]['fill_window'] = True
         response = self.client.post(reverse('api:request_groups-list'), data=good_data)
         rg = response.json()
-        self.assertEqual(rg['requests'][0]['configurations'][0]['instrument_configs'][0]['exposure_count'], 5)
-        self.assertEqual(response.status_code, 201)
-
-    def test_fill_window_when_exposures_fit_exactly(self):
-        good_data = self.generic_payload.copy()
-        good_data['requests'][0]['windows'][0] = {
-            'start': '2016-09-29T23:12:00Z',
-            'end': '2016-09-29T23:21:30Z'
-        }
-        good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['exposure_time'] = 10
-        good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['fill_window'] = True
-        # The largest interval is 570s, available time is 460s, and instrument config duration is 46s
-        n_exposures_fit_exactly = 10
-        response = self.client.post(reverse('api:request_groups-list'), data=good_data)
+        self.assertGreater(rg['requests'][0]['configurations'][0]['repeat_duration'],
+                           initial_repeat_duration)
+        self.assertEqual(rg['requests'][0]['configurations'][0]['repeat_duration'], 430.0)
         self.assertEqual(response.status_code, 201)
-        self.assertEqual(
-            response.json()['requests'][0]['configurations'][0]['instrument_configs'][0]['exposure_count'],
-            n_exposures_fit_exactly - 1
-        )
 
     def test_multiple_instrument_configs_with_different_rotator_modes_fails(self):
         bad_data = self.generic_payload.copy()