Skip to content

Minor fixes to modelbuilder class #452

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

butterman0
Copy link

@butterman0 butterman0 commented Apr 9, 2025

Made the following fixes:

  1. _validate_data method is called for predict_posterior method, but not predict method. Should be called in both as the only difference between the two should be whether mean or full posterior samples are returned.

  2. self.is_fitted_ = False set in class init, but not set to True when model is fitted with fit. Decided to keep the variable as it is useful, therefore set to True after 1) fit called 2) load with fitted data.

The alternative is to remove self.is_fitted_ altogether, seeing as the other functions tend to check for fitting via the existence of inference data (model.idata) instead. Alternatively, could refactor these checks to instead use is_fitted_.

@zaxtax
Copy link
Contributor

zaxtax commented Apr 12, 2025

Looks decent! Make the lint errors go away and the unit tests in model builder pass, and I think we can merge this

@butterman0
Copy link
Author

pre-commit.ci autofix

@butterman0
Copy link
Author

Thanks @zaxtax, got autofix to do it! Let me know if anything else.

@butterman0 butterman0 force-pushed the minor_fixes_to_modelbuilder_class branch from 8ffe9ce to 4d5a116 Compare May 19, 2025 12:16
@butterman0
Copy link
Author

Hi @zaxtax, I rebased as there seems to be some problems with pytest. Anything else I need to do here?

@butterman0 butterman0 force-pushed the minor_fixes_to_modelbuilder_class branch from 4d5a116 to 93c5ed6 Compare May 24, 2025 15:36
@butterman0
Copy link
Author

Hi @zaxtax, I removed the commit to add the call to _validate_data as I needed it in #426 to fix testing errors. There would have been some hacking going on otherwise. Not sure if this is good practice, but there won't be any conflicts now!

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