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

feat(ava/lite-insight): 新增多维度下钻波动归因方法 #658

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GuangMingYouBei
Copy link
Contributor

No description provided.

@@ -124,6 +124,7 @@ export type {
LowVarianceInfo,
CorrelationInfo,
InsightsResult,
DimensionDrillDownProps,
Copy link
Member

Choose a reason for hiding this comment

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

其他算子目前参数没有支持配置和导出类型,确实有打算支持(#643 ),不过参数命名应该需要统一下 xxOptions? cc @LAI-X

Copy link
Contributor Author

@GuangMingYouBei GuangMingYouBei Apr 27, 2023

Choose a reason for hiding this comment

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

改为了 options 的格式。

Copy link
Contributor

Choose a reason for hiding this comment

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

改为了 options 的格式。

这是改了之后的还是没改的😂
image

@@ -93,7 +93,7 @@ export type {
} from './data';

/* insight */
export { getInsights, generateInsightVisualizationSpec } from './insight';
export { getInsights, generateInsightVisualizationSpec, dimensionDrillDownAttribution } from './insight';
Copy link
Member

Choose a reason for hiding this comment

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

dimensionDrillDownAttribution 需要单独导出么,目前其他算法都是通过 getInsights 指定 insightType 导出的。单独导出 dimensionDrillDownAttribution 不太符合统一的调用形式

Copy link
Contributor Author

Choose a reason for hiding this comment

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

暂时去掉了单独导出,但是感觉这种算子是有可能单独调用的,以后再考虑怎么导出。

Copy link
Contributor

Choose a reason for hiding this comment

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

好像还是没去掉单独导出?
image

@@ -1,3 +1,5 @@
export { getInsights } from './pipeline';
export { generateInsightVisualizationSpec } from './pipeline/visualize';
export { dimensionDrillDownAttribution } from './insights/extractors/causalInference/dimensionDrillDown';
export type { DrillDownProps as DimensionDrillDownProps } from './insights/extractors/causalInference/types';
Copy link
Member

Choose a reason for hiding this comment

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

同上,确认下 dimensionDrillDownAttribution 是否可以不单独导出
另外 types 如果需要导出,写到 types 文件下,下面使用了 export * from './types' 导出 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.

去掉了

const tempResult = dimensionDrillDownAttribution(props);
const treeResult = tempResult.resultInTree;
expect(treeResult?.City.huangshishi.info.currValue).toBe(13722.76);
});
Copy link
Member

Choose a reason for hiding this comment

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

除了 currValue, 还有 diff, 贡献度等结果需要在测试用例中体现?


/** DataConfig specifies the input data with its focused dimensions and target measure to be analysed. */
export type DataConfig = {
sourceData: Datum[];
Copy link
Member

Choose a reason for hiding this comment

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

这些信息部分在 InsightOptions 里已经定义了,建议走 getInsights 整体的流程,不重复指标、维度这些参数

Copy link
Contributor Author

Choose a reason for hiding this comment

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

删掉了

};

/** Dimension drill down Result type */
export type DimensionDrillDownResult = {
Copy link
Member

Choose a reason for hiding this comment

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

类型命名和结果的结构整体改一下,和 insight 中其他算子类似,结果使用 BasePatternInfo 的命名和数据结构,可参考 TrendInfo

算法入参如 FluctInfo 可以统一叫 xxOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

targetMeasure: string;
}

export interface MeasureDecomposeProps extends CausalInferenceProps {
Copy link
Member

Choose a reason for hiding this comment

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

这个文件里有的地方用的 interface,有的用 type,不太统一
如果不是必须要用 interface,可以都用 type

@@ -0,0 +1,128 @@
import { cloneDeep as _cloneDeep } from 'lodash';
Copy link
Member

Choose a reason for hiding this comment

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

这个文件里的工具函数可以看下有没有可以放到 insight/utils 目录下面的(比较通用的可放)

measure: string,
fluctuationDim: string,
location: DataLocation
) => {
Copy link
Member

Choose a reason for hiding this comment

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

  • 这个函数参数有点多,建议用 object 管理,否则调用时不太方便一一对应上
  • 需要加上注释说明函数和每个参数含义,比如 item, dictFlatten, deque, location 都不是很好理解
  • 命名修改? dictFlatten --> flattenDict, fluctuationDim --> fluctuationDimension

globalDiff.diff = globalDiff.currValue - globalDiff.baseValue;

return { resultInTree: resultTree, globalDiff, resultInList: Object.values(DictFlatten) };
};
Copy link
Member

Choose a reason for hiding this comment

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

建议参考其他算子包一个 extractor function,这样外层可以用统一的方法调用各种类型的 insight extractor

@@ -0,0 +1,75 @@
import { dimensionDrillDownAttribution } from '../../../../../src/insight/insights/extractors/causalInference/dimensionDrillDown';
Copy link
Member

Choose a reason for hiding this comment

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

commit message : feat(ava/lite-insight) --> feat(ava/insight),且用英文写 commit message。
补充下 pr 描述

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants