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.

Sign up for GitHub

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

pass logger to filter for time consuming information #3296

Closed
wants to merge 3 commits into from
Closed

pass logger to filter for time consuming information #3296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48e400d
chore: pass logger to filter for time consuming information
jobo322 Nov 23, 2024
052f721
chore: update package-lock
jobo322 Nov 23, 2024
f2f6772
chore: stable release of nmr-processing
jobo322 Nov 23, 2024
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
6 changes: 3 additions & 3 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/component/EventsTrackers/KeysListenerTracker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { useAssignmentData } from '../assignment/AssignmentsContext.js';
import { useChartData } from '../context/ChartContext.js';
import { useDispatch } from '../context/DispatchContext.js';
import { useLoader } from '../context/LoaderContext.js';
import { useLogger } from '../context/LoggerContext.js';
import { usePreferences } from '../context/PreferencesContext.js';
import { useToaster } from '../context/ToasterContext.js';
import type { AlertButton } from '../elements/Alert.js';
Expand Down Expand Up @@ -83,6 +84,7 @@ function KeysListenerTracker(props: KeysListenerTrackerProps) {
return displayerMode === '1D' && data && data.length > 0;
}, [data, displayerMode]);

const { logger } = useLogger();
const deleteHandler = useCallback(
async (sourceData) => {
const { type, extra } = sourceData;
Expand Down Expand Up @@ -176,6 +178,7 @@ function KeysListenerTracker(props: KeysListenerTrackerProps) {
type: 'DELETE_EXCLUSION_ZONE',
payload: {
zone,
logger,
},
Copy link
Member

Choose a reason for hiding this comment

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

We need to ensure the reducer function remains pure. Passing the logger to the reducer introduces a side effect, which we should avoid. This issue has been consistently repeated across all the PRs you submitted

Copy link
Member

Choose a reason for hiding this comment

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

we should lift the function up that requires the logger as a parameter to the callback function and pass its result down to the reducer

Copy link
Member

Choose a reason for hiding this comment

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

We have a reducer that for example apply apodization on a spectrum. Apodization being done by nmr-processing.

We would like that nmr-processing can log some information (time spend in each filter) in the logger.

Problem is that the logger is changing another part of the state and this is not expected to be done in a reducer.

@targos Do you have some suggestions how to solve this issue ?

Copy link
Member

Choose a reason for hiding this comment

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

Hamed said it all. You need to do the processing outside of the reducer and send the result to the reducer. A reducer is not supposed to do heavy work. Just take some data and update the state with it.

});
hideLoading();
Expand All @@ -194,6 +197,7 @@ function KeysListenerTracker(props: KeysListenerTrackerProps) {
payload: {
zone,
spectrumId: spectrumID,
logger,
},
});
hideLoading();
Expand Down Expand Up @@ -288,6 +292,7 @@ function KeysListenerTracker(props: KeysListenerTrackerProps) {
toaster,
dispatchPreferences,
activeTab,
logger,
],
);

Expand Down
1 change: 1 addition & 0 deletions src/component/loader/useLoadFiles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ export function useLoadFiles(onOpenMetaInformation?: (file: File) => void) {
containsNmrium,
parseMetaFileResult,
spectraColors,
logger,
},
});
}
Expand Down
4 changes: 2 additions & 2 deletions src/component/main/NMRiumStateProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export default function NMRiumStateProvider(props: NMRiumStateProviderProps) {
},
});
}
dispatch({ type: 'INITIATE', payload: { nmriumState } });
dispatch({ type: 'INITIATE', payload: { nmriumState, logger } });
})
.catch((error: unknown) => {
dispatch({ type: 'SET_LOADING_FLAG', payload: { isLoading: false } });
Expand All @@ -129,7 +129,7 @@ export default function NMRiumStateProvider(props: NMRiumStateProviderProps) {
reportError(error);
});
}
}, [nmriumData, dispatch, dispatchPreferences]);
}, [nmriumData, dispatch, dispatchPreferences, logger]);
const { sortOptions } = useSortSpectra();

const spectra = useMemo(() => {
Expand Down
1 change: 1 addition & 0 deletions src/component/modal/LoadJCAMPModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import * as Yup from 'yup';

import { useDispatch } from '../context/DispatchContext.js';
import { useLogger } from '../context/LoggerContext.js';

Check failure on line 10 in src/component/modal/LoadJCAMPModal.tsx

View workflow job for this annotation

GitHub Actions / nodejs / lint-eslint 

'useLogger' is defined but never used
import { useToaster } from '../context/ToasterContext.js';
import { Input2Controller } from '../elements/Input2Controller.js';
import type { LabelStyle } from '../elements/Label.js';
Expand Down
4 changes: 3 additions & 1 deletion src/component/panels/MatrixGenerationPanel/MatrixGenerationPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type { MatrixFilter } from '../../../data/matrixGeneration.js';
import { getMatrixFilters } from '../../../data/matrixGeneration.js';
import { useChartData } from '../../context/ChartContext.js';
import { useDispatch } from '../../context/DispatchContext.js';
import { useLogger } from '../../context/LoggerContext.js';
import { usePreferences } from '../../context/PreferencesContext.js';
import type { GroupPaneStyle } from '../../elements/GroupPane.js';
import { GroupPane } from '../../elements/GroupPane.js';
Expand Down Expand Up @@ -124,6 +125,7 @@ function InnerMatrixGenerationPanel() {
activeTab,
);

const { logger } = useLogger();
const matrixOptions = getMatrixOptions(nucleusMatrixOptions.matrixOptions, {
from: originDomain.xDomain[0],
to: originDomain.xDomain[1],
Expand Down Expand Up @@ -152,7 +154,7 @@ function InnerMatrixGenerationPanel() {
function handleRemoveProcessing() {
dispatch({
type: 'DELETE_SPECTRA_FILTER',
payload: { filterName: signalProcessing.name },
payload: { filterName: signalProcessing.name, logger },
});
}

Expand Down
12 changes: 8 additions & 4 deletions src/component/panels/filtersPanel/Filters/FiltersSectionsPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { getFilterLabel } from '../../../../data/getFilterLabel.js';
import type { FilterEntry as BaseFilterEntry } from '../../../../data/types/common/FilterEntry.js';
import { useChartData } from '../../../context/ChartContext.js';
import { useDispatch } from '../../../context/DispatchContext.js';
import { useLogger } from '../../../context/LoggerContext.js';
import { useToaster } from '../../../context/ToasterContext.js';
import type { AlertButton } from '../../../elements/Alert.js';
import { useAlert } from '../../../elements/Alert.js';
Expand Down Expand Up @@ -62,14 +63,14 @@ function FilterElements(props: FilterElementsProps) {
} = props;
const { id, name, enabled } = filter;
const label = getFilterLabel(name);

const { logger } = useLogger();
function handleFilterCheck(id, event: React.ChangeEvent<HTMLInputElement>) {
const enabled = event.target.checked;
const hideLoading = toaster.showLoading({
message: `${enabled ? 'Enable' : 'Disable'} filter in progress`,
});
setTimeout(() => {
dispatch({ type: 'ENABLE_FILTER', payload: { id, enabled } });
dispatch({ type: 'ENABLE_FILTER', payload: { id, enabled, logger } });
hideLoading();
}, 0);
onEnableChange();
Expand All @@ -83,7 +84,10 @@ function FilterElements(props: FilterElementsProps) {
const hideLoading = await toaster.showAsyncLoading({
message: 'Delete filter process in progress',
});
dispatch({ type: 'DELETE_FILTER', payload: { id } });
dispatch({
type: 'DELETE_FILTER',
payload: { id, logger },
});
hideLoading();
},
intent: 'danger',
Expand All @@ -100,7 +104,7 @@ function FilterElements(props: FilterElementsProps) {
});
dispatch({
type: 'DELETE_SPECTRA_FILTER',
payload: { filterName: name },
payload: { filterName: name, logger },
});
hideLoading();
},
Expand Down
74 changes: 43 additions & 31 deletions src/component/reducer/actions/FiltersActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { v4 } from '@lukeed/uuid';
import type { NmrData1D, NmrData2DFt } from 'cheminfo-types';
import type { Logger, NmrData1D, NmrData2DFt } from 'cheminfo-types';
import type { Draft } from 'immer';
import { current } from 'immer';
import { xFindClosestIndex } from 'ml-spectra-processing';
Expand Down Expand Up @@ -148,12 +148,15 @@
>;
type EnableFilterAction = ActionType<
'ENABLE_FILTER',
{ id: string; enabled: boolean }
{ id: string; enabled: boolean; logger?: Logger }
>;
type DeleteFilterAction = ActionType<
'DELETE_FILTER',
{ id?: string; logger?: Logger }
>;
type DeleteFilterAction = ActionType<'DELETE_FILTER', { id?: string }>;
type DeleteSpectraFilterAction = ActionType<
'DELETE_SPECTRA_FILTER',
{ filterName: string }
{ filterName: string; logger?: Logger }
>;
type SetFilterSnapshotAction = ActionType<
'SET_FILTER_SNAPSHOT',
Expand All @@ -169,7 +172,7 @@
>;
type DeleteExclusionZoneAction = ActionType<
'DELETE_EXCLUSION_ZONE',
{ zone: ExclusionZone; spectrumId?: string }
{ zone: ExclusionZone; spectrumId?: string; logger?: Logger }
>;
type ApplySignalProcessingAction = ActionType<
'APPLY_SIGNAL_PROCESSING_FILTER',
Expand Down Expand Up @@ -258,6 +261,7 @@
key?: string | null;
activeSpectrum?: ActiveSpectrum | null;
triggerSource?: 'Apply' | 'none';
logger?: Logger;
}

function getFilterDomain(
Expand Down Expand Up @@ -296,6 +300,7 @@
reset = false,
key,
activeSpectrum,
logger,
triggerSource = 'none',
} = options || {};

Expand Down Expand Up @@ -330,19 +335,28 @@
});

if (isSpectrum1D(datum)) {
Filters1DManager.reapplyFilters(datum, filters);
Filters1DManager.reapplyFilters(datum, { filters, logger });
} else {
Filters2DManager.reapplyFilters(datum, filters);
Filters2DManager.reapplyFilters(datum, { filters, logger });
}

draft.tempData = current(draft).data;
// apply the current Filters
if (applyFilter) {
const { name, value } = datum.filters[filterIndex];
if (datum.info.dimension === 1) {
Filters1D[name].apply(datum, value);
const datum1D = datum as Spectrum1D;
Filters1DManager.applyFilter(
datum1D,
datum1D.filters[filterIndex],
logger,
);
} else {
Filters2D[name].apply(datum, value);
const datum2D = datum as Spectrum2D;
Filters2DManager.applyFilter(
datum2D,
datum2D.filters[filterIndex],
logger,
);
}
}

Expand All @@ -361,9 +375,9 @@
if (filterIndex === -1 || reset) {
if (draft.tempData) {
if (isSpectrum1D(datum)) {
Filters1DManager.reapplyFilters(datum);
Filters1DManager.reapplyFilters(datum, { logger });
} else {
Filters2DManager.reapplyFilters(datum);
Filters2DManager.reapplyFilters(datum, { logger });
}
}
//if the filter is not exists, create a clone of the current data
Expand Down Expand Up @@ -1271,15 +1285,15 @@
return;
}

const { id: filterID, enabled } = action.payload;
const { id: filterID, enabled, logger } = action.payload;
const datum = draft.data[activeSpectrum.index];

//apply filter into the spectrum
if (isSpectrum1D(datum)) {
Filters1DManager.enableFilter(datum, filterID, enabled);
Filters1DManager.enableFilter(datum, { id: filterID, enabled, logger });
}
if (isSpectrum2D(datum)) {
Filters2DManager.enableFilter(datum, filterID, enabled);
Filters2DManager.enableFilter(datum, { id: filterID, enabled, logger });
}

resetSelectedTool(draft);
Expand All @@ -1298,28 +1312,26 @@
}
}

function deleteFilter(datum: Spectrum, id?: string) {
function deleteFilter(datum: Spectrum, id?: string, logger?: Logger) {
const filters = datum.filters.slice(0);

let removedFilter;
if (!id) {
datum.filters = filters.filter((filter) =>

Check failure on line 1320 in src/component/reducer/actions/FiltersActions.ts

View workflow job for this annotation

GitHub Actions / nodejs / lint-check-types 

Type '(Filter1DEntry | Filter2DEntry)[]' is not assignable to type 'Filter1DEntry[] | Filter2DEntry[]'.
nonRemovableFilters.has(filter.name),
) as typeof filters;
);
} else {
removedFilter = datum.filters.find((filter) => filter.id === id);
datum.filters = filters.filter(
(filter) => filter.id !== id,
) as typeof filters;
datum.filters = filters.filter((filter) => filter.id !== id);

Check failure on line 1325 in src/component/reducer/actions/FiltersActions.ts

View workflow job for this annotation

GitHub Actions / nodejs / lint-check-types 

Type '(Filter1DEntry | Filter2DEntry)[]' is not assignable to type 'Filter1DEntry[] | Filter2DEntry[]'.
}

// do not reprocess the filters when the deleted filter is inactive
if (removedFilter && !removedFilter.enabled) return;

if (isSpectrum1D(datum)) {
Filters1DManager.reapplyFilters(datum);
Filters1DManager.reapplyFilters(datum, { logger });
} else {
Filters2DManager.reapplyFilters(datum);
Filters2DManager.reapplyFilters(datum, { logger });
}
}

Expand All @@ -1331,9 +1343,9 @@
return;
}

const filterID = action?.payload?.id;
const { id, logger } = action?.payload || {};
const datum = draft.data[activeSpectrum.index];
deleteFilter(datum, filterID);
deleteFilter(datum, id, logger);

draft.toolOptions.data.activeFilterID = null;
resetSelectedTool(draft);
Expand All @@ -1346,7 +1358,7 @@
draft: Draft<State>,
action: DeleteSpectraFilterAction,
) {
const filterName = action.payload.filterName;
const { filterName, logger } = action.payload;

if (draft.view.spectra.activeTab) {
for (const datum of draft.data) {
Expand All @@ -1358,11 +1370,11 @@

for (const filter of filtersResult) {
if (isSpectrum1D(datum)) {
Filters1DManager.deleteFilter(datum, filter.id);
Filters1DManager.deleteFilter(datum, filter.id, logger);
}

if (isSpectrum2D(datum)) {
Filters2DManager.deleteFilter(datum, filter.id);
Filters2DManager.deleteFilter(datum, filter.id, logger);
}
}
}
Expand Down Expand Up @@ -1497,7 +1509,7 @@
draft: Draft<State>,
action: DeleteExclusionZoneAction,
) {
const { zone, spectrumId } = action.payload;
const { zone, spectrumId, logger } = action.payload;

// if spectrum id exists, remove the selected exclusion zone in the spectrum
if (spectrumId) {
Expand All @@ -1510,10 +1522,10 @@
);
if (filter && isSpectrum1D(spectrum)) {
if (filter.value.length === 1) {
Filters1DManager.deleteFilter(spectrum, filter.id);
Filters1DManager.deleteFilter(spectrum, filter.id, logger);
} else {
filter.value = filter.value.filter((_zone) => _zone.id !== zone?.id);
Filters1DManager.reapplyFilters(spectrum);
Filters1DManager.reapplyFilters(spectrum, { logger });
}
}
} else {
Expand All @@ -1525,7 +1537,7 @@
filter.value = filter.value.filter(
(_zone) => zone.from !== _zone.from && zone.to !== _zone.to,
);
Filters1DManager.reapplyFilters(datum);
Filters1DManager.reapplyFilters(datum, { logger });
}
}
}
Expand Down
Loading
Loading