Skip to content
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

Clean up routing behaviour in ngram component #1727

Merged
merged 12 commits into from
Dec 23, 2024
4 changes: 2 additions & 2 deletions frontend/src/app/common-test-bed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { EntityService } from './services/entity.service';
import { WordmodelsService } from './services/wordmodels.service';
import { WordmodelsServiceMock } from '../mock-data/wordmodels';
import { VisualizationService } from './services/visualization.service';
import { visualizationServiceMock } from '../mock-data/visualization';
import { VisualizationServiceMock } from '../mock-data/visualization';
import { TagService } from './services/tag.service';
import { TagServiceMock } from '../mock-data/tag';
import { RouterStoreService } from './store/router-store.service';
Expand Down Expand Up @@ -72,7 +72,7 @@ export const commonTestBed = () => {
},
{
provide: VisualizationService,
useValue: new visualizationServiceMock(),
useValue: new VisualizationServiceMock(),
},
{
provide: TagService,
Expand Down
4 changes: 0 additions & 4 deletions frontend/src/app/models/field-filter.spec.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
import { convertToParamMap } from '@angular/router';
import { mockFieldMultipleChoice, mockFieldDate, mockField } from '../../mock-data/corpus';
import { EsDateFilter, EsTermsFilter } from './elasticsearch';
import { BooleanFilter, DateFilter, DateFilterData, MultipleChoiceFilter } from './field-filter';
import { of } from 'rxjs';
import { distinct } from 'rxjs/operators';
import { SimpleStore } from '../store/simple-store';
import { Store } from '../store/types';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change removes some unused imports, which I noticed when I was looking at other examples for testing routing behaviour.

Expand Down
47 changes: 47 additions & 0 deletions frontend/src/app/models/ngram.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { SimpleStore } from '../store/simple-store';
import { RouterStoreService } from '../store/router-store.service';
import { NgramParameters, NgramSettings } from './ngram';

describe('NgramParameters', ()=> {
let store: RouterStoreService = new SimpleStore() as any;
let ngramParameters: NgramParameters;
const testState = {
size: 3,
positions: 'first',
freqCompensation: true,
analysis: 'clean',
maxDocuments: 100,
numberOfNgrams: 20,
dateField: 'releaseDate'
} as NgramSettings;
const testParams = {ngramSettings: 's:3,p:first,c:true,a:clean,m:100,n:20,f:releaseDate'}

beforeEach(() => {
ngramParameters = new NgramParameters(store);
});

it('should correctly convert internal state to a route parameter', () => {
const params = ngramParameters.stateToStore(testState);
expect(params).toEqual(testParams);
});

it('should correctly convert a route parameter to an internal state', () => {
const state = ngramParameters.storeToState(testParams);
expect(state).toEqual(testState);
});

it('should return default values if no relevant route parameter is present', () => {
const defaultSettings = {
size: 2,
positions: 'any',
freqCompensation: false,
analysis: 'none',
maxDocuments: 50,
numberOfNgrams: 10,
dateField: 'date'
} as NgramSettings;
const state = ngramParameters.storeToState({irrelevant: 'parameter'})
expect(state).toEqual(defaultSettings);
})

});
64 changes: 64 additions & 0 deletions frontend/src/app/models/ngram.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import { Params } from '@angular/router';
import * as _ from 'lodash';

import { StoreSync } from '../store/store-sync';
import { Store } from '../store/types';

export interface NgramSettings {
size: number;
positions: string;
freqCompensation: boolean;
analysis: string;
maxDocuments: number;
numberOfNgrams: number;
dateField: string;
}

export class NgramParameters extends StoreSync<NgramSettings> {

keysInStore = ['ngramSettings'];

constructor(store: Store) {
super(store);
this.connectToStore();
}

stateToStore(state: NgramSettings): Params {
return { ngramSettings: [`s:${state.size}`,`p:${state.positions}`,`c:${state.freqCompensation}`,
`a:${state.analysis}`,`m:${state.maxDocuments}`,`n:${state.numberOfNgrams}`,
`f:${state.dateField}`].join(',') }
}

storeToState(params: Params): NgramSettings {
if (_.has(params, 'ngramSettings')) {
const stringComponents = params['ngramSettings'].split(',');
return {
size: parseInt(this.findSetting('s', stringComponents), 10),
positions: this.findSetting('p', stringComponents),
freqCompensation: this.findSetting('c', stringComponents) === 'true',
analysis: this.findSetting('a', stringComponents),
maxDocuments: parseInt(this.findSetting('m', stringComponents), 10),
numberOfNgrams: parseInt(this.findSetting('n', stringComponents), 10),
dateField: this.findSetting('f', stringComponents)
}
}
return {
size: 2,
positions: 'any',
freqCompensation: false,
analysis: 'none',
maxDocuments: 50,
numberOfNgrams: 10,
dateField: 'date'
Copy link
Contributor

Choose a reason for hiding this comment

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

The way the date field is handled looks a bit shaky, since the corpus may not actually have a field with this name. I think this was already an issue, though. Have you checked that the visualisation still works with corpora like parliament-denmark-old?

Copy link
Contributor Author

@BeritJanssen BeritJanssen Dec 20, 2024

Choose a reason for hiding this comment

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

Yes, good point. The problem is that when the ngramParameters model is initialized, the corpus input hasn't been set yet, so we cannot set the date field. Initializing in onChanges ... if (changes.corpus) and passing the first date field in the corpus on initialization may be the best way to solve this (in practice, this only happens once, as we will have to navigate away from the ngram component in order to change the corpus). I will try that. This highlights a problem that I see with outfactoring state management to models in general: often, state management and data manipulation requires transferring knowledge about preconditions for certain states as well. So a lot of the logic tied to the data will still be tied up in the component. This could probably be solved if we did more heavy refactoring, and not use @Input anymore to transfer corpus and queryModel state, and instead keep tabs on them through observables. Then again, my intuition is that it would move the code into a less angularesque state, and would make it harder to step into as a new developer.

Copy link
Contributor Author

@BeritJanssen BeritJanssen Dec 23, 2024

Choose a reason for hiding this comment

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

Hmm, I tried and abandoned the idea to initialize ngramParameters in ngOnChanges. That seems to work in practice, but unit testing depending on change detection is a bit of a nuisance. I decided to remove the date field from the routing parameters for now: the number of corpora in which different date fields are available is so limited that I think it's not too important to include the date field in the route at this moment. We can add it once we decided & implemented a more stable solution for transferring knowledge about corpus & queryModel between components and/or models.

} as NgramSettings;
}

findSetting(abbreviation: string, stringComponents: string[]): string | undefined{
const setting = stringComponents.find(s => s[0] === abbreviation);
return setting.split(':')[1];
}

getCurrentRouterState(): string {
return _.get(this.store.currentParams(), 'ngramSettings');
}
}
37 changes: 0 additions & 37 deletions frontend/src/app/models/visualization.spec.ts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only test in this spec was the one related to the ngram related models and types.

This file was deleted.

76 changes: 13 additions & 63 deletions frontend/src/app/models/visualization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,19 @@ export type WordcloudParameters = {
} & APIQuery;


export type NGramRequestParameters = {
corpus_name: string;
field: string;
ngram_size?: number;
term_position?: string;
freq_compensation?: boolean;
subfield?: string;
max_size_per_interval?: number;
number_of_ngrams?: number;
date_field: string;
} & APIQuery;


export interface FreqTableHeader {
key: string;
label: string;
Expand All @@ -96,69 +109,6 @@ export interface ChartParameters {
chartType: ChartType;
}

export type NGramRequestParameters = {
corpus_name: string;
field: string;
ngram_size?: number;
term_position?: string;
freq_compensation?: boolean;
subfield?: string;
max_size_per_interval?: number;
number_of_ngrams?: number;
date_field: string;
} & APIQuery;

export class NgramParameters {
size: number;
positions: string;
freqCompensation: boolean;
analysis: string;
maxDocuments: number;
numberOfNgrams: number;
dateField: string;

ngramSettings: string [];

constructor(size: number,
positions: string,
freqCompensation: boolean,
analysis: string,
maxDocuments: number,
numberOfNgrams: number,
dateField: string
) {
this.size = size;
this.positions = positions;
this.freqCompensation = freqCompensation;
this.analysis = analysis;
this.maxDocuments = maxDocuments;
this.numberOfNgrams = numberOfNgrams;
this.dateField = dateField;
}

toRouteParam(): string {
return [`s:${this.size}`,`p:${this.positions}`,`c:${this.freqCompensation}`,
`a:${this.analysis}`,`m:${this.maxDocuments}`,`n:${this.numberOfNgrams}`,
`f:${this.dateField}`].join(',');
}

fromRouteParam(paramString: string) {
this.ngramSettings = paramString.split(',');
this.size = parseInt(this.findSetting('s'), 10);
this.positions = this.findSetting('p');
this.freqCompensation = this.findSetting('c') === 'true';
this.analysis = this.findSetting('a');
this.maxDocuments = parseInt(this.findSetting('m'), 10);
this.numberOfNgrams = parseInt(this.findSetting('n'), 10);
this.dateField = this.findSetting('f');
}

findSetting(abbreviation: string): string | undefined{
const setting = this.ngramSettings.find(s => s[0] === abbreviation);
return setting.split(':')[1];
}
}

export interface FieldCoverage {
[field: string]: number;
};
6 changes: 3 additions & 3 deletions frontend/src/app/services/visualization.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ import {
GeoLocation,
MostFrequentWordsResult,
NGramRequestParameters,
NgramParameters,
QueryModel,
TaskResult,
TimeCategory,
} from '@models';
import { ApiService } from './api.service';
import { Observable } from 'rxjs';
import { NgramSettings } from '@models/ngram';

@Injectable({
providedIn: 'root'
Expand Down Expand Up @@ -96,7 +96,7 @@ export class VisualizationService {
corpus: Corpus,
queryModel: QueryModel,
field: string,
params: NgramParameters
params: NgramSettings
): NGramRequestParameters {
const query = queryModel.toAPIQuery();
return {
Expand All @@ -121,7 +121,7 @@ export class VisualizationService {
return this.apiService.getDateTermFrequency(params);
}

getNgramTasks(queryModel: QueryModel, corpus: Corpus, field: string, params: NgramParameters): Promise<TaskResult> {
getNgramTasks(queryModel: QueryModel, corpus: Corpus, field: string, params: NgramSettings): Promise<TaskResult> {
const ngramRequestParams = this.makeNgramRequestParameters(corpus, queryModel, field, params);
return this.apiService.ngramTasks(ngramRequestParams);
}
Expand Down
Loading
Loading