Fix CI failures due to domain transforms #108
Conversation
There was a problem hiding this comment.
Looks good functionality-wise!
Most of my comments relate to some confusion I have regarding what is going on in the code along with some minor design considerations. I think more inline explanations and docstrings could be very beneficial. There are a lot of new people on the project that could benefit from them.
If you disagree with any of my comments please let me know and we can discuss further.
src/blop/objectives.py
Outdated
| @property | ||
| def all_valid(self) -> bool: | ||
| return not (self.validity_conjugate_model and self.validity_probability) |
There was a problem hiding this comment.
Double checking intention on logic here, maybe worth adding it as a comment with explanation.
If either the validity_conjugate_model or the validity_probability are not defined, then all are valid. Why is this the case?
There was a problem hiding this comment.
The Objective gets its model from the _construct_model() method in the Agent, which gives it a validity_conjugate_model if it detects any invalid points (otherwise it's None). So if this attribute is not None then the points are all valid. But whether the points are all valid depend on the other objectives and the enforce_all_objectives_valid flag, so we have to do it in this sort of roundabout way.
|
I'll add some docstrings in the next PR, but this one should go out just to fix the bugs/CI failures. |
|
The plotting functions need to remove the |
|
I'll let this PR move forward and we can decide on whether the plotting functions should update models at a later time. Discussion in #109 |
This fixes #106, which was a subtle bug about data type mismatches on trust domain boundaries which introduced some
nans. While I was bug hunting I also cleaned up some other stuff, including the plotting code.