Add data parameter to _detect_relationships method
#2190
Conversation
sdv/metadata/multi_table.py
Outdated
| data = { | ||
| csv_file.stem: pd.read_csv(str(csv_file), **read_csv_parameters) | ||
| for csv_file in csv_files | ||
| } |
There was a problem hiding this comment.
should we just add the data to the dict in the for loop above?
sdv/metadata/multi_table.py
Outdated
| for csv_file in csv_files: | ||
| table_name = csv_file.stem | ||
| self.detect_table_from_csv(table_name, str(csv_file), read_csv_parameters) | ||
| data[csv_file.stem] = pd.read_csv(str(csv_file), **read_csv_parameters) |
There was a problem hiding this comment.
Can we just use the _load_from_csv util? Also instead of having to load the CSV twice, would it make more sense to load it and then use detect_from_dataframe instead?
There was a problem hiding this comment.
@frances-h Sure. Do you know if the detect_table_from_csv method is user facing? Otherwise this implementation removes the only usage of that method, so I would remove it.
There was a problem hiding this comment.
@amontanez24 thoughts? It's not in the docs but also doesn't have an _ preceding the function name. Maybe leave it for now and come back to it later?
There was a problem hiding this comment.
Yeah it's a private method, but you'd want to make sure it doesn't cause anything else to fail. I'm not sure if we use it anywhere else or in enterprise at all
There was a problem hiding this comment.
It's not used in SDV, SDV-Enterprise or SDGym.
There was a problem hiding this comment.
Then it should be fine to remove
fealho
force-pushed
the
enterprise-issue-734-foreign-key
branch
from
78efd86
to
bbede32
Compare
CU-86b0zfny6, Resolve https://github.com/datacebo/SDV-Enterprise/issues/643.