Added view to enable target selection for manual observing runs #840
Conversation
phycodurus
requested review from
phycodurus
and removed request for
phycodurus
There was a problem hiding this comment.
I've made some specific comments, that I think are important, but I have some general comments that I think will require a larger discussion.
Specifically, I'd like to talk about what you are actually looking to accomplish here. This returns a selection of targets in a list that have an airmass < 2.0 within a specific 24 hour window with 10 steps starting at YYYY-MM-DDT00:00:00 for a list of telescopes at a specific observatory. That feels like a highly specific use case that should probably be made more generalized. Users might want to change the limiting airmass, or see all of the telescopes at once, or set up their window so it encompasses a full night at CPT rather than splitting it.
Additionally, I feel like the information returned is limiting. This doesn't tell a user WHEN the target will be at it's lowest airmass, or sort them in any way or give a user an idea of how long the target will be up. Also, this is completely incompatible with non-sidereal targets.
Ultimately I'm wondering if this whole idea makes more sense being incorporated into the target list view. An airmass filter could possibly be added, and "observation planning" columns could be put on the table.
Would love to discuss further when you have time.
tom_targets/templates/tom_targets/target_facility_selection.html
Outdated
Show resolved
Hide resolved
|
I've implemented all of the changes requested above. I take your point about the potential for more generalized functionality for this view but I think it makes sense to implement this version first before trying to generalize. |
tom_targets/templates/tom_targets/target_facility_selection.html
Outdated
Show resolved
Hide resolved
| :alt: Target Selection table with additional columns added | ||
|
|
||
|
|
||
| Adding An Observing Facility to the Target Selection Form |
There was a problem hiding this comment.
I really don't like this section.
We really don't want people making orphaned facilities in this way.
I think we can add the basics for a DB driven general facility in a sprint.
I would recommend cutting this section and prioritizing that work in the upcoming sprint. It would be a much more sustainable way to do this, and will be necessary in the long run.
There was a problem hiding this comment.
I've removed this section, pending the other issue to add a General Facility model.
There was a problem hiding this comment.
Having now implemented the General Facility model, and integrated it with the target selection form, I've updated this section of the documentation accordingly.
|
Ideally I'd like this view to present the form, plus a paginated list of results when available, but this involves combining both a FormView to handle the POST and a ListView to handle the pagination. Pushing a partial refactor with most of the comments completed in order to get advice on the best way forward. |
… so as to cap the number of visibility calculations that can take place at a given time.
|
I've refactored the FormView to implement pagination on the list of targets, and updated the get and post methods so that pagination works as intended in the template. |
…ture/facility_target_selection
…calculation handling
|
I've integrated the new general facilities model with this form for calculating target visibilities from different observatories. The easiest way to handle this was to do some refactoring of the tom_observations.utils.get_sidereal_visibility function. |
| TargetList.objects.all(), | ||
| required=True) | ||
| facilities = [(x, x) for x in get_service_classes()] | ||
| facilities += [(x.full_name, x.full_name) for x in ObservingFacility.objects.all()] |
There was a problem hiding this comment.
these form fields are build very early on when running your tom, so this will break any TOM that doesn't have ObservingFacility model already in the DB before it can even run migrations.
You need to move the setting of the choice field to the __init__ below to prevent django attempting to evaluate the unknown model prematurely.
see TargetListShareForm above for an example.
| return redirect(reverse('tom_targets:list') + '?' + query_string) | ||
|
|
||
|
|
||
| class TargetGroupingView(PermissionListMixin, ListView): |
There was a problem hiding this comment.
I'm confused by this change.
This will break. There is no form included in the context.
| permission_required = 'tom_targets.view_targetlist' | ||
| template_name = 'tom_targets/target_grouping.html' | ||
| model = TargetList | ||
| paginate_by = 25 |
There was a problem hiding this comment.
We also require something in paginate_by since we call {% bootstrap_pagination page_obj extra=request.GET.urlencode %} in the template.
|
|
||
| # If the user did not select a general facility, but selected a facility module, | ||
| # this function should calculate for that facility alone. | ||
| # If the user selected neither a general facility or a facility module, calculate for the default |
| facilities = [clazz for name, clazz in facility.get_service_classes().items()] | ||
| elif observation_facility and general_facility is None: | ||
| facilities = [observation_facility] | ||
| else: |
There was a problem hiding this comment.
what is this else?
What condition leads to this?
| :param observation_facility: observing facility for which to calculate the airmass. None indicates all available | ||
| facilities. | ||
| :param observation_facility: observing facility declared as a facility class for which to calculate the airmass. |
There was a problem hiding this comment.
This is not what is passed to this function from other parts of the code. We pass a string. changing this here will break everywhere we submit strings for the facility name in the rest of tom_observations.
This feature aims to improve the TOM's useability for observers operating more traditional, block-scheduled observing facilities, often manually or through remote operations. The feature provides a form and view to enable them to easily identify targets visible at their facility on the date indicated.