-
Notifications
You must be signed in to change notification settings - Fork 1
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
Expose dataset loading functions as public API #68
Comments
This sounds like an easy improvement without any downsides to me! Would that get passed through
I don't see anything wrong with having dataset-related utils like this in the
I agree that there's utility to these functions for both other packages and end users, but I'm a little on the fence about the cost-benefit equation when it comes to making this work for the latter group. On the cost side, I see some complexity arising from a) probably needing to allow reading from paths as well as modules, and b) probably needing to validate data files to ensure they're compatible with the expected format. Meanwhile, the benefits seem less significant for end users. As I see it, a lot of the complexity of these functions comes from 1) avoiding a strict dependency on Let me know what you think! If I'm underestimating the value of these functions for end users, I'm not opposed to adding some complexity to make them more flexible. |
Yes, passing it through
Your entire argument makes perfect sense here. I agree that the complexity involved making it available for other users is not worth it when I'm happy to take a first cut at this, but happy to defer to you as well. |
Makes sense! We can always re-evaluate if we end up with more utils.
Sounds great! |
There are functions in
sknnr.datasets._base
that manage the loading of CSV files intoDataset
objects (similar to sklearn'sBunch
objects). At present, however, these utility functions (e.g._load_dataset_from_csv_filenames
and_load_csv_data
) are meant for internal use and are currently used inload_moscow_stjoes
andload_swo_ecoplot
. As @aazuspan points out in lemma-osu/synthetic-knn#2, these functions (and callees) rely on "specific restrictions like requiring an integer index in the first column, and floats in the remaining columns" (see here).Nonetheless, even with these restrictions, this functionality is likely useful across other packages that require
sknnr
or for users that may have similarly formatted CSV files. In order to use these functions in a public API, we would need to generalize_load_csv_data
so that it possibly acceptsDATA_MODULE
as a keyword argument (as insklearn.datasets.load_csv_data
). This would work well for other packages that want to use this functionality, although perhaps users might have to create a module structure with their data to use it as well.To consider:
sknnr.datasets
or in a more generic "utilities" module?importlib.resources
) or from generic directories as well?@aazuspan, further thoughts on this?
The text was updated successfully, but these errors were encountered: