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

Bug fixes for the FACTS method #533

Merged
merged 6 commits into from
Jul 5, 2024
Merged

Conversation

phantom-duck
Copy link
Contributor

@hoffmansc

Wrote fixes for the bugs documented in issues #531 and #532.

For the issue of the hard-coded features in the recIsValid function (issue #531, code here), the fix currently proposed is a hotfix. The values have not been removed, only the relevant argument's default value has been changed from True to False.

Whether you would like us to remove this part of the code altogether or keep working on it to make it more robust (and if so, in this pull request or a new one), please let me know.

Fixes #531 and #532.

Signed-off-by: phantom-duck <37215464+phantom-duck@users.noreply.github.com>
this part of the code is problematic due to the use of hardcoded feature
names. The required functionality should be achieved in some other way.

Signed-off-by: phantom-duck <37215464+phantom-duck@users.noreply.github.com>
Signed-off-by: phantom-duck <37215464+phantom-duck@users.noreply.github.com>
Signed-off-by: phantom-duck <37215464+phantom-duck@users.noreply.github.com>
Copy link
Collaborator

@hoffmansc hoffmansc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is good for now. For #531, it would be good to have a more robust system eventually. For #532, can we add a short test case like in the issue to show it's working?

Thanks!

@phantom-duck
Copy link
Contributor Author

Hello! Thanks for the feedback. Apologies for the delayed response.

For #531, it would be good to have a more robust system eventually

Ok, great, we will see to it as soon as possible and create a new PR.

For #532, can we add a short test case like in the issue to show it's working?

Of course. So this one I will add to this PR right?

@hoffmansc
Copy link
Collaborator

Of course. So this one I will add to this PR right?

Yes, please 🙂

Signed-off-by: phantom-duck <37215464+phantom-duck@users.noreply.github.com>
Previously, the test case only had inf costs. Consequently, the
exact values of the feature weights were not actually tested properly.

Signed-off-by: phantom-duck <37215464+phantom-duck@users.noreply.github.com>
@phantom-duck
Copy link
Contributor Author

Hello again @hoffmansc !

I have now added a test showcasing that the costs are computed correctly (close to the example in #532). Just a question to be sure I understood correctly, is this what you meant? Or by "test case" you mean also to add something to the example notebook for example?

Other than, I think we are good to go.

@hoffmansc
Copy link
Collaborator

This is perfect, thanks!

@hoffmansc hoffmansc merged commit e011686 into Trusted-AI:main Jul 5, 2024
9 checks passed
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.

FACTS method bug: code in valid_ifthens function has hard-coded feature names
2 participants