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

Read/save bins Issue #96 #277

Merged
merged 17 commits into from
Jan 9, 2024
Merged

Read/save bins Issue #96 #277

merged 17 commits into from
Jan 9, 2024

Conversation

alexliap
Copy link
Contributor

@alexliap alexliap commented Nov 5, 2023

This PR aims to resolve Issue #96 by adding to class methods to OptimalBinning object, to_json & read_json.
Continuous and Multiclass binning inherit from that class, so no other methods were needed.
The to_json method also saves the transformation of the training data as requested in the Issue.

@guillermo-navas-palencia
Copy link
Owner

Hi @alexliap.

Thanks for taking the time to develop this PR. However, this is not what I had in mind (sorry if the description was lacking details). The json file should contain all the information needed to build the binning table.

@alexliap
Copy link
Contributor Author

alexliap commented Dec 6, 2023

Thank you for the response.
I will work on it, but could you be a little more specific in order to avoid refactoring again?

@alexliap
Copy link
Contributor Author

What about now, i save all the data required for the binning table, but is this the way you want it to behave? should the self._check_is_fitted() change in order to be able to call self.binning_table instead of self._binning_table ?

@alexliap alexliap changed the base branch from master to develop December 22, 2023 15:00
Commited by mistake
Commited by mistake
@guillermo-navas-palencia
Copy link
Owner

Yes, this looks good to me. Regarding self._is_fitted, yes, I think it should be set to True when reading from JSON.

@guillermo-navas-palencia
Copy link
Owner

guillermo-navas-palencia commented Dec 23, 2023

To complete the PR, it would be nice to have unit tests for all binning classes. I mention this because the binning table classes require different arguments depending on the target type, so continuous and multiclass targets will not work automatically.

@alexliap
Copy link
Contributor Author

Yes of course, I'm on it

@alexliap
Copy link
Contributor Author

uuum question, why MulticlassOptBinning doesn't require dtype of variable as input?

@guillermo-navas-palencia
Copy link
Owner

uuum question, why MulticlassOptBinning doesn't require dtype of variable as input?

MulticlassOptBinning only accepts numerical dtype.

@guillermo-navas-palencia
Copy link
Owner

Thanks!

@guillermo-navas-palencia guillermo-navas-palencia merged commit 1a74eb5 into guillermo-navas-palencia:develop Jan 9, 2024
12 checks passed
@alexliap alexliap deleted the read_save_bins branch January 9, 2024 09:41
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.

2 participants