Skip to content

Conversation

@yshady
Copy link

@yshady yshady commented Jul 5, 2024

No description provided.

@yshady yshady requested a review from a team as a code owner July 5, 2024 19:02
@yshady
Copy link
Author

yshady commented Jul 8, 2024

@microsoft-github-policy-service agree

Copy link
Author

@yshady yshady left a comment

Choose a reason for hiding this comment

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

SMAC max iterations too low and float value enforcements to avoid a '' crash

@motus
Copy link
Member

motus commented Jul 8, 2024

a few things:

  • There are pylint and pycodestyle violations. Run all these checks locally to make sure the code conforms to our coding standards.
  • There are many changes that are unrelated to the fix (indentation, parameter updates, etc.). We usually split them out into separate PRs.
  • Most importantly, I doubt that we need that fix at all. We somehow got some garbage in the database, and it is completely legitimate that we complain about it now. I think the right solution would be to clean up the DB and remove the offending records (and investigate how they've got there) instead of quietly coercing data to NaNs.

@bpkroth, what do you think?

@motus motus assigned motus and unassigned motus Jul 8, 2024
@eujing
Copy link
Contributor

eujing commented Jul 10, 2024

@motus the float fixes @yshady are proposing are due to this issue described here in more detail: #785

The fix proposed here (some kind of string to float conversion) should probably be applied earlier, before self._adjust_signs_df is called. In this case, we will just get a whole series of NaNs when bulk register is called.

@motus
Copy link
Member

motus commented Jul 10, 2024

Thanks for identifying the issue! @eujing has filed an issue #785 describing the problem. Proper fix submitted in PR #789

@motus motus closed this Jul 10, 2024
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.

3 participants