ConfigSpace
ConfigSpace copied to clipboard
Fix small bugs related to nan values in vector passed to pdf
I tried to incorporate the new prior system with SMAC and noticed some bugs regarding NaN values introduced by conditional hyperparameters. This PR resolves all issues I have encountered with conditional hyperparameters.
If you would like to, I can also provide a minimal working example showcasing the current problems.
Codecov Report
Base: 67.97% // Head: 67.97% // No change to project coverage :thumbsup:
Coverage data is based on head (
eb021c4
) compared to base (88051eb
). Patch has no changes to coverable lines.
Additional details and impacted files
@@ Coverage Diff @@
## main #256 +/- ##
=======================================
Coverage 67.97% 67.97%
=======================================
Files 25 25
Lines 1786 1786
=======================================
Hits 1214 1214
Misses 572 572
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Hi @Ennosigaeon,
Sorry for the delay! A minimal working example would be great to illuminate the needs for the change, thanks for the PR! I'll review it once I can see what it aims to fix
Hey, I just found some time to finally rework this PR. There are now two changes, each in a separate commit and with according unit tests.
- It is currently not possible to call ConfigSpace.substitute_hyperparameters_in_* with either InConditions or ForbiddenRelations. The first commit resolves this issue.
- If you are using ConfigSpaces with conditional hyperparameters, some config values in the array representation may be nan. In the current SMAC implementation of user priors all values, including potential nans, are passed to _pdf. This commit returns a PDF of 0 for nan values.
Regarding the minimal working example: You can use the changed unit tests to provoke exceptions with the current main.
Amazing, thanks for the update, I will check this once I'm back next week!
Yes, your assumption is correct. An additional values
field is introduced but value
also exists.
Hey @Ennosigaeon, any updates on this? It would be great if we could merge this PR as the number of merge issues is increasing.
@mfeurer: Sorry, I forgot about this issue. I can take a look at in February.
Hey, sorry to bring this up again, but we're starting to move hyperparameters into individual files in #292. It would be great if we could merge this one before to avoid extensive merge conflicts..
I am on vacation this week. I will take a look at it beginning of next week. Promised ;)
Ok, everything looks fine from my side. I will just ignore the failed tests due to The job was not started because recent account payments have failed or your spending limit needs to be increased. Please check the 'Billing & plans' section in your settings.
Please let me know if you want me to change anything else.
Thanks a lot for these updates. Indeed, github appears to have hickups and this is not due to this PR.
Thanks a lot. Github has resolved the hickups, so I just merged your contribution.