Skip to content
This repository has been archived by the owner on Aug 21, 2023. It is now read-only.

Feature/awf/cleanup time agg param#212 #231

Merged
merged 4 commits into from
Sep 1, 2017
Merged

Feature/awf/cleanup time agg param#212 #231

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8f95a8c
Refactor IndicatorQueryOpts
Aug 30, 2017
d65bc42
Refactor ExtraParams event emitters
Aug 31, 2017
b0a45a1
Refactor to time_aggregation: TimeAggParam
Aug 31, 2017
64ecb69
Refactor Indicator related model names
Sep 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 9 additions & 12 deletions src/app/charts/chart.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ import { ChartData } from '../models/chart-data.model';
import { City } from '../models/city.model';
import { ClimateModel } from '../models/climate-model.model';
import { IndicatorQueryOpts } from '../models/indicator-query-opts.model';
import { ThresholdIndicatorQueryOpts } from '../models/threshold-indicator-query-opts.model';
import { IndicatorParams } from '../models/indicator-params.model';
import { Scenario } from '../models/scenario.model';
import { TimeAggParam } from '../models/time-agg-param.enum';

import { AuthService } from '../auth/auth.service';
import { ChartService } from '../services/chart.service';
Expand All @@ -42,14 +43,14 @@ import * as _ from 'lodash';
export class ChartComponent implements OnChanges, OnDestroy, AfterViewInit {

@Output() onRemoveChart = new EventEmitter<Chart>();
@Output() onExtraParamsChanged = new EventEmitter<any>();
@Output() onExtraParamsChanged = new EventEmitter<IndicatorParams>();

@Input() chart: Chart;
@Input() scenario: Scenario;
@Input() models: ClimateModel[];
@Input() city: City;
@Input() unit: string;
@Input() extraParams;
@Input() extraParams: IndicatorParams;

private processedData: ChartData[];
public chartData: ChartData[];
Expand Down Expand Up @@ -115,18 +116,15 @@ export class ChartComponent implements OnChanges, OnDestroy, AfterViewInit {
this.cancelDataRequest();
}

updateChart(extraParams: any) {
updateChart(extraParams: IndicatorParams) {
this.cancelDataRequest();
this.chartData = [];
this.rawChartData = [];

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not understanding why the typecheck doesn't complain when ThresholdIndicatorParams are sent through

Copy link
Author

@CloudNiner CloudNiner Sep 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because interface ThresholdIndicatorParams extends interface IndicatorParams

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah interesting. Extending works the opposite direction I was expecting, then. Cool.

let params = {
let params: IndicatorParams = {
climateModels: this.models,
unit: this.unit || this.chart.indicator.default_units,
// TODO: #212
// As a temporary solution, the time agg defaults to the 1st valid option.
// Really, this should a user selectable option
time_aggregation: this.chart.indicator.valid_aggregations[0]
time_aggregation: TimeAggParam.Yearly
}

params = _.extend(params, this.extraParams);
Expand Down Expand Up @@ -181,9 +179,8 @@ export class ChartComponent implements OnChanges, OnDestroy, AfterViewInit {
this.imageExportService.downloadAsPNG(this.chart.indicator.name, fileName);
}

public onThresholdSelected($event) {
const thresholdParams = $event.data as ThresholdIndicatorQueryOpts;
this.extraParams = thresholdParams;
public onThresholdSelected(params: IndicatorParams) {
this.extraParams = params;
this.onExtraParamsChanged.emit(this.extraParams);
this.updateChart(this.extraParams);
}
Expand Down
15 changes: 7 additions & 8 deletions src/app/charts/extra-params-components/threshold.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { AfterViewInit, Component, EventEmitter, Input, Output, OnInit } from '@
import { FormBuilder, FormGroup, Validators } from '@angular/forms';

import { Indicator } from '../../models/indicator.model';
import { ThresholdIndicatorParams } from '../../models/threshold-indicator-params.model';

import * as _ from 'lodash';

Expand All @@ -16,7 +17,8 @@ import * as _ from 'lodash';
export class ThresholdComponent implements AfterViewInit, OnInit {

@Input() indicator: Indicator;
@Input() extraParams: any;
@Input() extraParams: ThresholdIndicatorParams;
@Output() thresholdParamSelected = new EventEmitter<ThresholdIndicatorParams>();

thresholdForm: FormGroup;

Expand Down Expand Up @@ -50,8 +52,6 @@ export class ThresholdComponent implements AfterViewInit, OnInit {

@Input() thresholdUnits: any[] = this.thresholdTemperatureUnits;

@Output() thresholdParamSelected = new EventEmitter<any>();

constructor(private formBuilder: FormBuilder) {}

ngOnInit() {
Expand All @@ -62,11 +62,11 @@ export class ThresholdComponent implements AfterViewInit, OnInit {
ngAfterViewInit() {
// Since valueChanges triggers initially before parent is ready, wait until
// parent is ready here and trigger it to draw chart with extra parameters.
this.thresholdParamSelected.emit({data: {
this.thresholdParamSelected.emit({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the data object removed here? You had asked for it to be added in another PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I had misunderstood what the original problem was based on the comments before, combined with forgetting the details of how EventEmitters work. What's here now is the proper way to return data via event emitters, if you do not need the DOM Event

'threshold_comparator': this.thresholdForm.controls.comparatorCtl.value,
'threshold': this.thresholdForm.controls.thresholdCtl.value,
'threshold_units': this.thresholdForm.controls.thresholdUnitCtl.value
}});
});
}

createForm() {
Expand All @@ -78,12 +78,11 @@ export class ThresholdComponent implements AfterViewInit, OnInit {
});

this.thresholdForm.valueChanges.debounceTime(700).subscribe(form => {
this.thresholdParamSelected.emit({data: {
'event': event,
this.thresholdParamSelected.emit({
'threshold_comparator': form.comparatorCtl,
'threshold': form.thresholdCtl,
'threshold_units': form.thresholdUnitCtl
}});
});
});
}

Expand Down
3 changes: 2 additions & 1 deletion src/app/lab/lab.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { ProjectService } from '../services/project.service';

import { Chart } from '../models/chart.model';
import { Indicator } from '../models/indicator.model';
import { IndicatorParams } from '../models/indicator-params.model';
import { Project } from '../models/project.model';


Expand Down Expand Up @@ -81,7 +82,7 @@ export class LabComponent implements OnInit, OnDestroy {
this.indicator = null;
}

public saveExtraParams(params: any) {
public saveExtraParams(params: IndicatorParams) {
this.project.project_data.extraParams = params;
}

Expand Down
3 changes: 2 additions & 1 deletion src/app/models/chart-data.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@ import { Indicator } from './indicator.model';
import { City } from './city.model';
import { ClimateModel } from './climate-model.model';
import { Scenario } from './scenario.model';
import { TimeAggParam } from './time-agg-param.enum';

/* tslint:disable:variable-name */
export class ChartData {
indicator: Indicator;
data: MultiDataPoint[];
time_aggregation: string;
time_aggregation: TimeAggParam;
time_format: string;
city?: City;
climate_models?: ClimateModel[];
Expand Down
10 changes: 10 additions & 0 deletions src/app/models/indicator-params.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { ClimateModel } from './climate-model.model';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now there are two models:
indicator-parameters.model.ts
indicator-params.model.ts

in the models folder, and that is confusing

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any suggestions for better names? otherwise I'll try to come up with something

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been using the term "extra params"
Maybe
indicator-extra-params?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or perhaps indicator-query-parameters

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have indicator-query-opts and these params aren't always the subset that are the extra params. They're really the GET params to an API request. What about the following refactor:

  • IndicatorQueryOpts -> IndicatorRequestOpts
  • IndicatorParams -> IndicatorGetParams
  • Leave IndicatorParam unchanged

Copy link
Contributor

@fungjj92 fungjj92 Sep 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK IndicatorRequestOpts
2 -- > IndicatorParamsList? or IndicatorQueryStringParams?
OK IndicatorParam

Copy link
Author

@CloudNiner CloudNiner Sep 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IndicatorQueryParams would just be a shorter version of your second option that isn't any less descriptive. How about that? Definitely like your second suggestion, which is more properly named than even IndicatorGetParams, than the first.

import { TimeAggParam } from './time-agg-param.enum';

export interface IndicatorParams {
climateModels?: ClimateModel[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed these are all optional. Shouldn't they be required?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because none of these params are required to make an API request, they all have defaults.

years?: string[];
time_aggregation?: TimeAggParam;
agg?: string;
unit?: string;
}
8 changes: 2 additions & 6 deletions src/app/models/indicator-query-opts.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,12 @@
import { City } from './city.model';
import { Indicator } from './indicator.model';
import { IndicatorParams } from './indicator-params.model';
import { ClimateModel } from './climate-model.model';
import { Scenario } from './scenario.model';

export interface IndicatorQueryOpts {
indicator: Indicator;
city: City;
scenario: Scenario;
params: {
climateModels?: ClimateModel[];
years?: string[];
time_aggregation?: string;
unit?: string;
}
params: IndicatorParams;
}
7 changes: 7 additions & 0 deletions src/app/models/threshold-indicator-params.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { IndicatorParams } from './indicator-params.model';

export interface ThresholdIndicatorParams extends IndicatorParams {
threshold: Number;
threshold_units: string;
threshold_comparator: string;
}
16 changes: 0 additions & 16 deletions src/app/models/threshold-indicator-query-opts.model.ts
View file
Open in desktop

This file was deleted.

6 changes: 6 additions & 0 deletions src/app/models/time-agg-param.enum.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@

export enum TimeAggParam {
Yearly = 'yearly',
Quarterly = 'quarterly',
Monthly = 'monthly'
}
1 change: 0 additions & 1 deletion src/app/services/chart.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ export class ChartService {

private timeOptions = {
'yearly': '%Y',
'daily': '%Y-%m-%d',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a quarterly format added here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we don't use it but it wouldn't hurt to be thorough

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, can't do this now. It would require additional refactoring. In the quarterly case, we need to convert the keys in the API (e.g. 2010-3) into the start date for a "quarter" which would be July 1, 2010 for 2010-3, and that can't be done with D3.timeParse. We'd need this refactor for pretty much all of the other remaining valid time aggs if we chose to display them in the lab UI.

'monthly': '%Y-%m'
};

Expand Down
12 changes: 6 additions & 6 deletions src/app/services/indicator.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import 'rxjs/Rx';

import { Indicator } from '../models/indicator.model';
import { IndicatorQueryOpts } from '../models/indicator-query-opts.model';
import { ThresholdIndicatorQueryOpts } from '../models/threshold-indicator-query-opts.model';
import { ThresholdIndicatorParams } from '../models/threshold-indicator-params.model';
import { ApiHttp } from '../auth/api-http.service';
import { apiHost } from '../constants';

Expand All @@ -30,14 +30,14 @@ export class IndicatorService {

// append extra parameters, if needed
if (isThresholdIndicator(options.indicator.name)) {
const thresholdOpts: ThresholdIndicatorQueryOpts = <ThresholdIndicatorQueryOpts> options;
const thresholdParams = options.params as ThresholdIndicatorParams;
// abort request if chart is in flux (these parameters are required)
if (!thresholdOpts.params.threshold) {
if (!thresholdParams.threshold) {
return Observable.of({url: ''});
}
searchParams.append('threshold', thresholdOpts.params.threshold.toString());
searchParams.append('threshold_units', thresholdOpts.params.threshold_units);
searchParams.append('threshold_comparator', thresholdOpts.params.threshold_comparator);
searchParams.append('threshold', thresholdParams.threshold.toString());
searchParams.append('threshold_units', thresholdParams.threshold_units);
searchParams.append('threshold_comparator', thresholdParams.threshold_comparator);
}

if (options.params.years) {
Expand Down