Higher order syntactic and semantic profiler transforms #653
Conversation
Signed-off-by: aishwariyachakraborty <aishwariya.chakraborty@gmail.com>
Signed-off-by: aishwariyachakraborty <aishwariya.chakraborty@gmail.com>
aishwariyachakraborty
force-pushed
the
syntactic-semantic-profiler
branch
from
eab62a5
to
230c500
Compare
Signed-off-by: aishwariyachakraborty <aishwariya.chakraborty@gmail.com>
Signed-off-by: aishwariyachakraborty <aishwariya.chakraborty@gmail.com>
There was a problem hiding this comment.
please merge your branch with dev and regenerate this file.
There was a problem hiding this comment.
I have made all the changes you have requested. I do not have an option to merge yet. It shows : Merging is blocked
Merging can be performed automatically once the requested changes are addressed.
There was a problem hiding this comment.
The merge I'm referring to is one you do locally into your fork.
There was a problem hiding this comment.
please merge your branch with dev and regenerate this file.
transforms/code/higher_order_syntactic_profiler/python/pyproject.toml
Outdated
Show resolved
Hide resolved
transforms/code/higher_order_syntactic_profiler/ray/pyproject.toml
Outdated
Show resolved
Hide resolved
1. Updated makefiles to new the format used in noop 2. Updated author names in toml file Signed-off-by: aishwariyachakraborty <aishwariya.chakraborty@gmail.com>
|
I have made all the changes you have requested. I do not have an option to merge yet. It shows : Merging is blocked |
|
|
||
| | Parameter | Default | Description | | ||
| |------------|----------|--------------| | ||
| | `HOSP_METRICS_LIST` | `CCR` | Metrics to be calculated for profiling. Multiple metrics can be entered separated by space. Only valid metric is `CCR` as of now. | |
There was a problem hiding this comment.
I believe the key needs to be in lower case, per the transform implementation. And, the transform seems to look for metrics not metrics_list and definitely not hosp_metrics_list.
| the options provided by | ||
| the [python launcher](../../../../data-processing-lib/doc/python-launcher-options.md). | ||
| ``` | ||
| --hosp_metrics_list HOSP_METRICS_LIST |
There was a problem hiding this comment.
Can you use https://github.com/IBM/data-prep-kit/blob/dev/transforms/language/pdf2parquet/python/README.md as a template for documenting the configurations and annotations. In particular, you haven't discussed the annotations that are added, but following the format for the configuration would be good too.
| input parquet to the output folder, without modification. | ||
| """ | ||
| self.logger.debug(f"Transforming one table with {len(table)} rows") | ||
| if self.metrics_list is not None: |
There was a problem hiding this comment.
You should check for this error/condition in the initializer and throw an exception there if a bad value, such as None.
There was a problem hiding this comment.
It appears your are adding a column whose name is the name of the metric. This is not clear from README.
There was a problem hiding this comment.
if you really want to distribute. these, maybe they belong in a data directory that is a sibling of src and then include them in the wheel via the toml file. My problem with this is that I'm wondering how someone would specify this file (or other data files) when using the wheel and not the repo project. If they can be referenced from within the wheel, it would be better to specify data/ikb/ikb_model.csv than src/ikb/ikb_model.csv I think.
| "pipeline": "pipeline_id", | ||
| "job details": { | ||
| "job category": "preprocessing", | ||
| "job name": "NOOP", |
There was a problem hiding this comment.
this file needs to be regenerated since it seems to have been copied from the NOOP project.
There was a problem hiding this comment.
@aishwariyachakraborty: Please slack me when you have change to discuss - Internal ID touma@us.ibm.com
|
It was suggested that we combine this with the Syntactic Construct Extractor transform and create a single transform. So I am closing this PR. I will create a new PR after the PR for Syntactic Construct Extractor transform is merged with the dev branch. Hope this is ok? |
Why are these changes needed?
This PR implements the higher order syntactic and semantic profiler transforms as a downstream application of the UBSR transform.
Related issue number (if any).